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

Fixing the cluster spec output formatting issue #724

Closed
wants to merge 1 commit into from
Closed

Fixing the cluster spec output formatting issue #724

wants to merge 1 commit into from

Conversation

viggy28
Copy link

@viggy28 viggy28 commented Oct 24, 2019

To address #723.

@sgotti
Copy link
Member

sgotti commented Oct 25, 2019

@viggy28 Thanks for your PR! Can you rework the commit message and description with something more descriptive like:

stolonctl: don't show null clusterspec options

make the clusterspec struct fields tagged as json `omitempty` so they will be omitted when null marshaled.
This won't have any side effect on clusterdata writing but will make stolonctl don't show them.

@viggy28
Copy link
Author

viggy28 commented Oct 25, 2019

Hi @sgotti, thanks for looking. Updated it.

@sgotti
Copy link
Member

sgotti commented Oct 28, 2019

@viggy28 Thinking and testing this PR I noticed that it will break the stolonctl spec --defaults output hiding some defaults values.

One possible solution will require the creation (in stolonctl_spec.go), of two different cluster spec structs (identical to the original cluster spec, one with all json tags with omitempty and one without omitempty) that could be converted from the original clusterspec so their marshalling will work for both cases (this will also help detecting when we forgot to add new fields to all the structs at compilation time).

@viggy28
Copy link
Author

viggy28 commented Oct 30, 2019

Thanks @sgotti. Updated the PR. Let me know if that works.

@@ -290,6 +290,86 @@ type ClusterSpec struct {
AutomaticPgRestart *bool `json:"automaticPgRestart"`
}

type ClusterSpecNew struct {
Copy link
Member

Choose a reason for hiding this comment

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

There's the need of two different cluster spec structs identical to the original cluster spec:

  • one with all json tags with omitempty for normal output
  • one with all json tags without omitempty for --default output

@@ -290,6 +290,86 @@ type ClusterSpec struct {
AutomaticPgRestart *bool `json:"automaticPgRestart"`
}

type ClusterSpecNew struct {
// Interval to wait before next check
Copy link
Member

Choose a reason for hiding this comment

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

Just remove all the fields comments.

Copy link
Author

Choose a reason for hiding this comment

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

updated.

make the clusterspec struct fields tagged as json `omitempty` so they will be omitted when null marshaled.
This won't have any side effect on clusterdata writing but will make stolonctl don't show them.
@sgotti
Copy link
Member

sgotti commented Nov 27, 2019

One possible solution will require the creation (in stolonctl_spec.go), of two different cluster spec structs (identical to the original cluster spec, one with all json tags with omitempty and one without omitempty) that could be converted from the original clusterspec so their marshalling will work for both cases (this will also help detecting when we forgot to add new fields to all the structs at compilation time).

@viggy28 Sorry if I wasn't clear but I meant to use two new structs inside stolonctl/cmd/spec.go, take a look at this compare:

https://github.com/sorintlab/stolon/compare/master...sgotti:stolonctl_spec_improve_output?expand=1

@sgotti
Copy link
Member

sgotti commented Dec 2, 2019

@viggy28 Thanks for your PR, I created a PR #736 with an updated implementation.

@sgotti sgotti closed this Dec 2, 2019
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.

3 participants