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

Empty get addons returns empty json/yaml array instead of empty string #4922

Merged
merged 6 commits into from
Mar 15, 2022

Conversation

gdlx
Copy link
Contributor

@gdlx gdlx commented Mar 9, 2022

Description

Returns empty json/yaml array/map instead of an empty strings when sending empty slice/array/map to Json/Yaml printers.

I had to call make to avoid Marshall to return null. I turned empty objects into string array/map as they're empty anyway.

Fixes #4910

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes
  • (Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@gdlx gdlx force-pushed the fix-empty-get-addons branch from f247e24 to aa9c508 Compare March 10, 2022 14:15
@@ -92,8 +92,15 @@ func getAddon(cmd *cmdutils.Cmd, params *getCmdParams) error {

if len(summaries) == 0 {
logger.Info("no addons found")
return nil
if params.output == "json" || params.output == "yaml" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this whole logic should be shifted into the printer. This has the added problem that any kind of additional change will affect this little logic differently. If we wrap the output into something we'll have to remember to update this. Just shift the whole decision into the printer and then call the printer. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've moved the logic into the printers.
I didn't find a simple way not to repeat the code.
Also, I create a string empty slice, as we know it's empty anyway.
And I've also fixed maps, after checking they're facing the same bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to add some printer tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some tests and updated the PR description.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gdlx Thank you for doing this!

Copy link
Contributor

@aclevername aclevername left a comment

Choose a reason for hiding this comment

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

lgtm!

@aclevername aclevername requested review from a team and Skarlso March 11, 2022 12:19
Copy link
Contributor

@Skarlso Skarlso left a comment

Choose a reason for hiding this comment

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

lgtm! nice job, well done for figuring out a nice way to do this. :)

@gdlx gdlx force-pushed the fix-empty-get-addons branch from e6fd330 to a1064f4 Compare March 14, 2022 09:23
@nikimanoledaki nikimanoledaki enabled auto-merge (squash) March 15, 2022 08:00
@nikimanoledaki nikimanoledaki disabled auto-merge March 15, 2022 08:00
@nikimanoledaki nikimanoledaki changed the title Empty get addons now returns empty json/yaml array instead of empty string Empty get addons returns empty json/yaml array instead of empty string Mar 15, 2022
@nikimanoledaki nikimanoledaki enabled auto-merge (squash) March 15, 2022 08:02
@nikimanoledaki nikimanoledaki merged commit 3bc088d into eksctl-io:main Mar 15, 2022
@gdlx gdlx mentioned this pull request Apr 5, 2022
7 tasks
@hspencer77 hspencer77 mentioned this pull request Jul 8, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty get addons as Json output should be empty array
5 participants