Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sync): Always print summary #197

Merged
merged 2 commits into from
Aug 7, 2020
Merged

Conversation

cwurm
Copy link
Contributor

@cwurm cwurm commented Aug 5, 2020

When decK encounters an error (e.g. invalid plugin configuration) it will currently not print the summary, making it hard to know what was still changed despite the error.

This small change makes sure the summary is printed even in that case and adds an Errors: field when there was at least one error.

Sample output:

$ ./deck sync -s .
creating consumer jwt-user
...
...
...
creating plugin openid-connect for route 8e4bf008-c470-4456-b333-7094c7089070
Summary:
  Created: 112
  Updated: 15
  Deleted: 0
  Errors: 2

Error: 2 errors occurred:
	while processing event: {Create} failed: 400 Bad Request {"message":"2 schema violations (only one of these fields must be non-empty: 'config.aws_region', 'config.host'; config.aws_region1: unknown field)","name":"schema violation","fields":{"config":{"aws_region1":"unknown field"},"@entity":["only one of these fields must be non-empty: 'config.aws_region', 'config.host'"]},"code":2}
	while processing event: {Create} failed: 400 Bad Request {"message":"3 schema violations (only one of these fields must be non-empty: 'config.allow', 'config.deny'; at least one of these fields must be non-empty: 'config.allow', 'config.deny'; config.whitelist1: unknown field)","name":"schema violation","fields":{"config":{"whitelist1":"unknown field"},"@entity":["only one of these fields must be non-empty: 'config.allow', 'config.deny'","at least one of these fields must be non-empty: 'config.allow', 'config.deny'"]},"code":2}

@cwurm cwurm requested a review from hbagdi August 5, 2020 18:48
if errs != nil {
printErr := color.New(color.FgRed, color.Bold).PrintfFunc()
printErr(" Errors: %v\n\n", len(errs))
return utils.ErrArray{Errors: errs}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Printing and returning error will result in printing of errors twice.
I'd recommend returning the error and not printing the error here as returning the error does two things:

  • print the error
  • make the deck unix process exit with a non-zero exit code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we don't have to print the errors in the summary. I removed it - now the existing code has just changed order.

@hbagdi hbagdi merged commit f8155c4 into Kong:main Aug 7, 2020
@cwurm cwurm deleted the always_summarize branch August 10, 2020 09:12
rainest pushed a commit that referenced this pull request Apr 21, 2021
When decK encounters an error (e.g. invalid plugin configuration) it will currently
not print the summary,
making it hard to know what was still changed despite the error.

This patch makes sure that the summary is printed even during errors.

From #197
AntoineJac pushed a commit that referenced this pull request Jan 23, 2024
When decK encounters an error (e.g. invalid plugin configuration) it will currently
not print the summary,
making it hard to know what was still changed despite the error.

This patch makes sure that the summary is printed even during errors.

From #197
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants