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

bundle validate: new alpha flag --output=<text|json-alpha1> to format results #3011

Merged
merged 4 commits into from
May 28, 2020
Merged

bundle validate: new alpha flag --output=<text|json-alpha1> to format results #3011

merged 4 commits into from
May 28, 2020

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented May 12, 2020

Description of the change:
Alpha hidden flag. See:

  • With success:

Screenshot 2020-05-24 at 11 36 04

  • With Error:

Screenshot 2020-05-24 at 11 35 57

@camilamacedo86 camilamacedo86 added kind/feature Categorizes issue or PR as related to a new feature. olm-integration Issue relates to the OLM integration labels May 12, 2020
@camilamacedo86 camilamacedo86 requested a review from estroz May 13, 2020 14:22
cmd/operator-sdk/bundle/output.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/output.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/output.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/output.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/cmd.go Outdated Show resolved Hide resolved
@camilamacedo86
Copy link
Contributor Author

HI @estroz,

Thank your for your review and great suggestions. All are addressed with 1 exception only as described above.

@camilamacedo86 camilamacedo86 requested a review from estroz May 14, 2020 19:50
@jmrodri

This comment has been minimized.

@jmccormick2001

This comment has been minimized.

@jmrodri

This comment has been minimized.

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

To me this seems like a large code change for effectively just calling json.Marshal on the result of the validation.

cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 changed the title feat: Add new hidden alpha flag --json to output the result of operator-sdk bundle validate in JSON format. feat: Add new hidden alpha flag --output to output the result of operator-sdk bundle validate in JSON format. May 14, 2020
@camilamacedo86

This comment has been minimized.

@camilamacedo86 camilamacedo86 requested a review from joelanford May 14, 2020 23:24
cmd/operator-sdk/bundle/output.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/output.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/output.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/output.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 requested a review from estroz May 15, 2020 19:06
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Show resolved Hide resolved
Copy link
Contributor Author

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

@joelanford @estroz,

I changed the impl to use the dep instead of we impl the output structure which makes the thing easier and cleaner. See:

Screenshot 2020-05-17 at 16 19 09

Let me know what do you think and if we can move forward with now.

@camilamacedo86 camilamacedo86 deleted the alpha-output-format branch May 19, 2020 09:52
@camilamacedo86 camilamacedo86 restored the alpha-output-format branch May 19, 2020 15:01
@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented May 22, 2020

Hi @estroz,

After all possibilities and suggestions, it is now updated and done on with your ideal one. Please, feel free to check. :-) Relly tks for the help, I think that it is much better now following your approach. 🥇

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

Getting there, although the JSON spec should be internal to the SDK (for now).

cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
pkg/output/logger.go Outdated Show resolved Hide resolved
pkg/output/result.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 24, 2020
@camilamacedo86 camilamacedo86 requested a review from estroz May 24, 2020 16:09
@camilamacedo86
Copy link
Contributor Author

Hi @estroz,

All is done here, could you please give a hand in its review?
Really tks.

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

Structure is getting there.

cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.sum Outdated Show resolved Hide resolved
internal/output/logger.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
cmd/operator-sdk/bundle/validate.go Outdated Show resolved Hide resolved
@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented May 27, 2020

Hi @estroz,

All done as suggested

Just here: #3011 (comment) I do not move with your comment because I thought that would be better re-check first since it would mean shows all validation tests have completed successfully with errors and warnings. However, if you think that is fine because it would mean that the command finishes then, I am OK about doing this change. Just let me know.

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

A few nits then LGTM

@camilamacedo86 camilamacedo86 requested a review from estroz May 28, 2020 00:39
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 28, 2020
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 28, 2020
@estroz estroz changed the title feat: Add new hidden alpha flag --output to output the result of operator-sdk bundle validate in JSON format. bundle validate: new alpha flag --output=<text|json-alpha1> to format results May 28, 2020
@estroz estroz merged commit d1dbfc8 into operator-framework:master May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. olm-integration Issue relates to the OLM integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants