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.

Sign up for GitHub

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

Jump to bottom

Add JSON status (fix #956) #958

Merged
merged 1 commit into from
Feb 10, 2023
Merged

Add JSON status (fix #956) #958

merged 1 commit into from
Feb 10, 2023

Conversation

raphink
Copy link
Member

@raphink raphink commented Jul 6, 2022 •
edited by squeed
Loading

  • feat: add json output for clustermesh status
  • feat: add json output for status
  • feat: downcase json export fields for status
  • feat: downcase json export fields for clustermesh status

Fixes: #956

@raphink raphink temporarily deployed to ci July 6, 2022 16:08 Inactive
aditighag
aditighag requested changes Jul 6, 2022
View reviewed changes
internal/cli/cmd/clustermesh.go Outdated Show resolved Hide resolved
status/status.go Outdated Show resolved Hide resolved
@raphink raphink temporarily deployed to ci July 7, 2022 05:42 Inactive
@raphink raphink temporarily deployed to ci July 7, 2022 05:45 Inactive
@raphink
Copy link
Member Author

raphink commented Jul 11, 2022

@aditighag is it fine like this?

@raphink raphink requested review from a team as code owners August 8, 2022 09:59
@raphink raphink requested a review from ldelossa August 8, 2022 09:59
@raphink raphink temporarily deployed to ci August 8, 2022 09:59 Inactive
@sayboras sayboras self-requested a review August 8, 2022 10:01
squeed
squeed reviewed Sep 1, 2022
View reviewed changes
internal/cli/cmd/status.go Outdated Show resolved Hide resolved
aditighag
aditighag reviewed Sep 1, 2022
View reviewed changes
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the revisions. Can you please squash the commits where you renamed compact -> summary, and the commit with omitempty additions?

I didn't take a look at the clustermesh changes closely as Tam is already in the reviewer's list.

internal/cli/cmd/clustermesh.go Outdated Show resolved Hide resolved
sayboras
sayboras approved these changes Sep 4, 2022
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

I have couple of small comments as per below, but the changes are LGTM in general 💯

internal/cli/cmd/status.go Outdated Show resolved Hide resolved
clustermesh/clustermesh.go Show resolved Hide resolved
@raphink raphink temporarily deployed to ci September 8, 2022 08:53 Inactive
@raphink raphink requested a review from a team as a code owner September 8, 2022 10:27
@raphink raphink requested review from rolinh and removed request for a team September 8, 2022 10:27
@raphink raphink temporarily deployed to ci September 8, 2022 10:32 Inactive
@rolinh rolinh added the needs-rebase This PR needs to be rebased because it has merge conflicts. label Sep 23, 2022
@raphink raphink temporarily deployed to ci January 13, 2023 16:07 — with GitHub Actions Inactive
@raphink
Copy link
Member Author

raphink commented Jan 13, 2023

I think I've addressed everything (I've given up on the enum because it clashed with the option declaration).

@raphink raphink temporarily deployed to ci January 13, 2023 16:43 — with GitHub Actions Inactive
@raphink raphink temporarily deployed to ci January 13, 2023 21:27 — with GitHub Actions Inactive
@raphink raphink temporarily deployed to ci January 16, 2023 10:53 — with GitHub Actions Inactive
tklauser
tklauser approved these changes Jan 17, 2023
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

LGTM, only minor nits inline regarding where OutputJSON and OutputSummary are defined. Once these are addressed and all commits squashed into one, this is ready to merge. Thanks a lot!

internal/cli/cmd/clustermesh.go Outdated Show resolved Hide resolved
clustermesh/clustermesh.go Outdated Show resolved Hide resolved
internal/cli/cmd/status.go Outdated Show resolved Hide resolved
internal/cli/cmd/status.go Outdated Show resolved Hide resolved
squeed
squeed reviewed Jan 17, 2023
View reviewed changes
clustermesh/clustermesh.go Outdated Show resolved Hide resolved
squeed
squeed reviewed Jan 17, 2023
View reviewed changes
clustermesh/clustermesh.go Outdated Show resolved Hide resolved
squeed
squeed reviewed Jan 17, 2023
View reviewed changes
internal/cli/cmd/status.go Outdated Show resolved Hide resolved
@raphink raphink temporarily deployed to ci January 17, 2023 13:53 — with GitHub Actions Inactive
@raphink raphink temporarily deployed to ci January 17, 2023 13:57 — with GitHub Actions Inactive
@raphink raphink temporarily deployed to ci February 6, 2023 16:17 — with GitHub Actions Inactive
tklauser
tklauser reviewed Feb 7, 2023
View reviewed changes
internal/cli/cmd/status.go Outdated Show resolved Hide resolved
@raphink raphink temporarily deployed to ci February 7, 2023 18:12 — with GitHub Actions Inactive
@tklauser tklauser temporarily deployed to ci February 10, 2023 13:00 — with GitHub Actions Inactive
@tklauser
Copy link
Member

@raphink I've squashed all the commits, rebased on top of latest master and made two additional changes (#958 (comment) and #958 (comment)) and force pushed to this PR's branch. Could you please do a quick sanity check on whether this still matches what you had in mind? I think then we're finally good to merge this.

@raphink @tklauser
feat: add json output for status and clustermesh status
ff97a32 
Signed-off-by: Raphaël Pinson <raphael@isovalent.com>
@tklauser tklauser temporarily deployed to ci February 10, 2023 13:09 — with GitHub Actions Inactive
@tklauser tklauser added the priority/release-blocker This issue will prevent the release of the next version of Cilium. label Feb 10, 2023
@tklauser tklauser merged commit 44acf5a into cilium:master Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser approved these changes

@squeed squeed squeed left review comments

@sayboras sayboras sayboras approved these changes

@ldelossa ldelossa ldelossa approved these changes

@aditighag aditighag Awaiting requested review from aditighag

@rolinh rolinh Awaiting requested review from rolinh rolinh was automatically assigned from cilium/vendor

Assignees
No one assigned
Labels
priority/release-blocker This issue will prevent the release of the next version of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JSON output for status
7 participants
@raphink @tklauser @squeed @ldelossa @sayboras @aditighag @rolinh