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

Show diff in 'config profile' commands, add dry-run flag #4923

Closed
wants to merge 2 commits into from

Conversation

preetsinojiya
Copy link

Issue: #4914

package jsondiff has been added to gx and is used to show user the difference between the original config and config that will be generated when config profile apply --dry-run is run

License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Preet <[email protected]>
@Stebalien
Copy link
Member

I couldn't fetch the copy of jsondiff that you imported so I re-imported it (QmTGd4vj3ZjBj8zY7aBYpp7LN3L2DiXbhd8YRLYu9papZk) and pinned it.

strDiff := string(byteDiff[:])

fmt.Println("Defference between current config and config that will be generated :\n ")
fmt.Println(strDiff)
Copy link
Member

Choose a reason for hiding this comment

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

We use a server-client model for our commands so, instead of printing here, we'll want to send the result back to the user (using res.SetOutput(diff)).

I'd send the output of jsondiff.Compare and then format it on the client in an Encoder/Marshaler.


We actually have two commands system. An old one (that we're slowly replacing) and a new one. Unfortunately, this command uses the old system. If you're up to it, I'd consider migrating it to the new system as that'll make this extension a bit easier.

If you do migrate it, you'll need to define an Encoder like we do in the patchAppendDataCmd command in core/commands/object/patch.go. If you don't want to deal with migrating it, you'll have to define a marshaler as is done in the version command in core/commands/version.go.

Copy link
Member

Choose a reason for hiding this comment

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

One open question is: Should we always return the diff and make --dry-run just mean "don't apply the change"? Personally, I'd find that useful. @Kubuxu or @kevina (or anyone) thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

One open question is: Should we always return the diff and make --dry-run just mean "don't apply the change"

I think that's what we'd want

@magik6k magik6k changed the title Issue4914 Show diff in 'config profile' commands, add dry-run flag Apr 18, 2018
@whyrusleeping whyrusleeping added the need/author-input Needs input from the original author label Jun 13, 2018
@Stebalien
Copy link
Member

Continued in #5455.

@Stebalien Stebalien closed this Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/author-input Needs input from the original author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants