feature Implementation: Diff between Revisions #6 #22

codenio · 2018-01-24T19:41:16Z

This provides the ability to do a diff on revisions.
I have made some improvements with

safer code terminations with (exit 0),
Intelligent suggestions for subcommands
docs
code restructuring.
kindly have a look, and let me know in case of corrections/bugs.

Thanks..!!

codenio · 2018-01-26T12:44:59Z

@databus23 , any update on this..?

databus23 · 2018-01-26T12:47:29Z

@aananthraj Ah, I'm sorry I'm swamped at work at the moment. I set myself a reminder to take a look at this later today.

codenio · 2018-01-26T13:38:52Z

Thanks @databus23, for the prompt response.

I have one more feature request and its implementation in a separate branch feature/rollback-diff for forecasting what a helm rollback would change (revert). My question here is, Can I raise a new pull request ? or include those as part of this pull request ?.

Thanks,
Aananth K

databus23

Thanks for working on this! I've gotten similar requests in the past and never got around to tackle this. I'm very much willing to get this in this time.

I would like to discuss my general remark about the command structure before digging deeper into the code as quite a bit will change if we decide to take a different approach.

databus23 · 2018-01-29T15:09:23Z

README.md

+
+Available Commands:
+  revision    Shows diff between revision's manifests
+  upgrade     visualize changes, that a helm upgrade could perform


Restructring the command in the proposed way would be backward incompatible. I would very much would like to avoid that. Or at minimum find a clever way to alias the old CLI behaviour.
Otherwise this PR will break existing scripts and pipelines where helm is used.

I'm still liking the idea to maintain a single command helm diff [FROM] [TO] where FROM and TO are more generic.

Something like this:

# latest revision with local chart helm diff RELEASE local-dir/ #latest revision with latest chart from stable helm diff RELEASE stable/postgresql #revision 12 with latest revison helm diff RELEASE@12 #revision 12 with revision 14 helm diff RELEASE@12 @14

What do you think?

Restructring the command in the proposed way would be backward incompatible.

It is an valid point, that we have to discuss. Those changes were proposed for the following reasons

To have resemblance with helm's usage patterns/generic usage pattern
eg:

# diff on upgrade helm upgrade [RELEASE] [CHART] [flags] helm diff upgrade [RELEASE] [CHART] [flags] # diff on rollback (available as a separate pull request #24) helm rollback [flags] [RELEASE] [REVISION] helm diff rollback [flags] [RELEASE] [REVISION] # diff on revision helm diff revision [flags] [RELEASE] REVISION

Features can be modular, clean and would be easy add new features.
For instance rollback

In case these points are not appealing, we could work on it.

hi @databus23,
any updates/suggestions on this?

I couldn't understand the below statement

Otherwise this PR will break existing scripts and pipelines where helm is used

could you elaborate so that it will be helpful for me to fix.

Moreover, I couldn't find any breaking scripts and pipelines while installing the plugin. kindly correct me if am wrong.

Thanks

sstarcher · 2018-04-16T11:51:55Z

@aananthraj I believe the discussions here might be of help - #34 (comment)

helm diff upgrade was added, but helm diff is aliased to helm diff upgrade

The CLI has been restructured so you can now add revision support if you have time to refactor.

sfrique · 2018-04-16T21:04:40Z

I had a small problem trying to build it, then I see the new release.

So I will wait for the refactor to test it.

Sorry, I current have very limited time.

codenio · 2018-04-20T05:33:36Z

@sstarcher thanks for update 👍 , was waiting for this..!!!

codenio · 2018-04-20T13:02:21Z

Hi @databus23 and @sstarcher ,

made suitable changes to include subcommands like

revision

$ helm diff revision -h

This command compares the manifests details of a named release.

It can be used to compare the manifests of 

- lastest REVISION with specified REVISION
 $ helm diff revision [flags] [RELEASE] [REVISION]

- REVISION1 with REVISION2
 $ helm diff revision [flags] [RELEASE] [REVISION1] [REVISION2]

Usage:
 diff revision [flags] [RELEASE] REVISION1 REVISION2

Flags:
   -h, --help   help for revision

rollback

$ helm diff rollback -h

This command compares the laset manifests details of a named release 
with specific revision values to rollback.

It forecasts/visualizes changes, that a helm rollback could perform.

Usage:
  diff rollback [flags] [RELEASE] [REVISION]

Flags:
  -h, --help   help for rollback

opinions..?

codenio · 2018-04-23T05:01:07Z

can this be merged..? or still some fixes needed..?

codenio · 2018-04-25T07:33:27Z

I would be happy, if we could close this before 27/04/14, beyond which it would be difficult for me to fix issues with this PR, as I am planning for long break. Hope no issues exist.
@sstarcher can you help here..?
@databus23 opinions..?

databus23

Thanks for the PR. In general this looks good and I would like to accept it. I made some suggestions in how to cleaner separate the commands and maybe remove some duplication.

databus23 · 2018-04-25T08:42:04Z

cmd/revision.go

+`
+
+func revisionCmd() *cobra.Command {
+	diff := diffCmd{}


I would prefer if this could use its own struct and not piggy back on the exiting struct. The general Idea would be that every command has its own struct, to store state and functions.

each subcommand is given a client, as off now both has resemblance, but it could change in feature.

# cmd/revision.go type revision struct { release string client helm.Interface suppressedKinds []string revisions []string }

# cmd/rollback.go type rollback struct { release string client helm.Interface suppressedKinds []string revisions []string }

databus23 · 2018-04-25T08:43:05Z

cmd/revision.go

+				return errors.New("Too many arguments to Command \"revision\".\nMaximum 3 arguments allowed: release name, revision1, revision2")
+			}
+
+			if q, _ := cmd.Flags().GetBool("suppress-secrets"); q {


Maybe we can move this to a global flag?

In my point of view, for executing suppress-secrets as a global pre-run , we might need to have a common diff object across all the subcommands and it must be made available in root.go

opinions on this..? can this be done in some other way..?

Hmm, you are right. Haven't thought of that, maybe leave it as it is for now. I think its more important to have the command cleanly separated. We can deduplicate later.

databus23 · 2018-04-25T08:43:23Z

cmd/revision.go

+				diff.suppressedKinds = append(diff.suppressedKinds, "Secret")
+			}
+
+			if nc, _ := cmd.Flags().GetBool("no-color"); nc {


This can be a global flag for sure, that is shared by all sub commands. And have the effect for done in a Pre function once for all commands.

we could use PersistentPreRunE,
when we add those flag checks PersistentPreRunE in the root command, all its children commands (upgrade,revision,rollback) will inherit and execute.

I think for this flag its a good fit as it just flips a package level var

Works fine for me.., hope this is ok

func New() *cobra.Command { chartCommand := newChartCommand() cmd := &cobra.Command{ Use: "diff", Short: "Show manifest differences", //Alias root command to chart subcommand Args: chartCommand.Args, // parse the flags and check for actions like suppress-secrets, no-colors PersistentPreRun: func(cmd *cobra.Command, args []string) { if nc, _ := cmd.Flags().GetBool("no-color"); nc { ansi.DisableColors(true) } }, RunE: func(cmd *cobra.Command, args []string) error { cmd.Println(`Command "helm diff" is deprecated, use "helm diff upgrade" instead`) return chartCommand.RunE(cmd, args) }, } // add no-color as global flag cmd.PersistentFlags().Bool("no-color", false, "remove colors from the output") // add flagset from chartCommand cmd.Flags().AddFlagSet(chartCommand.Flags()) cmd.AddCommand(newVersionCmd(), chartCommand) // add subcommands cmd.AddCommand( revisionCmd(), rollbackCmd(), ) cmd.SetHelpCommand(&cobra.Command{}) // Disable the help command return cmd }

databus23 · 2018-04-25T08:44:48Z

plugin.yaml

@@ -5,6 +5,6 @@ version: "2.8.2+2"
 usage: "Preview helm upgrade changes as a diff"
 description: "Preview helm upgrade changes as a diff"
 useTunnel: true
-command: "$HELM_PLUGIN_DIR/bin/diff"
+command: "$HELM_PLUGIN_DIR/diff"


this conflicts with master and needs to be reverted.

resolved it

databus23 · 2018-04-25T12:11:27Z

cmd/root.go

 	}

-	chartCommand := newChartCommand()


Huh, why remove all this? The aliasing is done to keep backward compatibility. I would like to keep that for a while to make that this doesn't break existing CI systems.

oops...!! I didn't remove it. just rearranged the code syntatically.
It is there, just fitted chartCommand.Args it inside, cobra.command and placed chartCommand := newChartCommand() above it

chartCommand := newChartCommand() cmd := &cobra.Command{ Use: "diff", Short: "Show manifest differences", //Alias root command to chart subcommand Args: chartCommand.Args,

does this need changes/reverting..?, its exactly the same code..!!

Ah, ok my Mistake, thanks for clearing this up. LGTM

databus23 · 2018-04-26T08:30:30Z

cmd/upgrade.go

@@ -20,6 +19,7 @@ type diffCmd struct {
 	reuseValues     bool
 	resetValues     bool
 	suppressedKinds []string
+	revisions       []string


this is unused now IMO

databus23 · 2018-04-26T11:43:47Z

cmd/revision.go

+func revisionCmd() *cobra.Command {
+	diff := revision{}
+	revisionCmd := &cobra.Command{
+		Use:   "revision [flags] [RELEASE] REVISION1 REVISION2",


When doing some manually tests I was wondering if revision [flags] RELEASE REVISION1 [REVISION2] would be more correct?

both revision1 and revision2 are mandatory, so how can it be indicated..?

It seems to me like REVISION2 is not mandatory and defaults to the latest revision. At least I can do helm diff revision $RELEASE 42.
I think RELEASE1 could even be also be optional and default to $latest-1. So that a simple helm diff revision would give you the latest change applied.

databus23 · 2018-04-26T11:52:42Z

I've tested this a bit and I'm quite happy with how it works. I think we are almost done.

I noticed that both commands are missing the --suppress and --suppres-secrets flag (They are not shown when running helm diff revison|rollback -h). Can we add them?

Other then that the last remaining bit would be to squash and rebase this so that it can be cleanly merged.

codenio · 2018-04-26T13:11:29Z

Thanks @databus23 for guiding me all the way..!!

Other then that the last remaining bit would be to squash and rebase this so that it can be cleanly merged.

one help, I have never squash multiple commits into a single one before. can u help..?
i tried something and i am not sure, whether it is correct

sstarcher · 2018-04-26T13:14:28Z

git rebase -i HEAD~4 where 4 is the number of commits to combine

databus23 · 2018-04-26T14:10:30Z

Something is messy with the branch. It look fine from the overall changes but can't be merged cleanly in master.

databus23 · 2018-04-26T14:26:43Z

I'm not a git guru. For me this very much applies: https://xkcd.com/1597/

I think we can keep this PR if you manage to force push the aananthraj:feature/revision-diffbranch with a clean commit on top of master.

Here is how I would fix it:

git checkout -b feature/revision-diff-new master
git diff master..feature/revision-diff | patch -p1
git commit -a -m "..."
git push -f aananthraj feature/revision-diff

This is dry coded, might not work but something like this should do the job.

codenio · 2018-04-26T14:48:02Z

reopening it in a cleaner way

databus23 · 2018-04-26T15:47:31Z

Looks to me like you lost some stuff during your git spelunking.

databus23 · 2018-04-26T16:42:09Z

This PR is now missing all of the code changes.
Its just the README. As I said you lost something during your git advanture... :)

codenio · 2018-04-26T17:34:43Z

all in one commit ❤️

super clean 😎
contains code 😆
working fine for me 👍
no merge conflicts 😄

@databus23 can we..?
waiting for it..!!

databus23 · 2018-04-26T21:54:54Z

Thanks for you hard work on this!

databus23 · 2018-04-26T22:00:35Z

@aananthraj I just noticed that your README changes are missing from this PR. Would you mind re-adding them in a separate commit.

codenio · 2018-04-27T00:09:53Z

sure, tried adding it again in this PR, but looks like the branch is locked after its merge. solution would be

create a separate PR
Revert the current merge and add the commit here and merge it again.

the former look feasible to me. will create a new PR.

@databus23 Thanks a lot 🙏 , it took a very long time to make this feature live.
feeling awesome now. Thanks for making me learn cool stuff about GitHub 😆

sfrique · 2018-04-27T01:30:11Z

I've tested a few revision diffs and a upgrade diff and it seems to be working!
helm diff helps a lot to verify changes before applying them, now it will help to see whats changed over the time.
Thanks a lot for this feature.

Readme and Usage Updation : following #22

databus23 reviewed Jan 29, 2018

View reviewed changes

codenio mentioned this pull request Feb 7, 2018

optimising bash scripts. zjj2wry/helm-diff#1

Merged

codenio changed the title ~~feature Implementing: Diff between Revisions #6~~ feature Implementation: Diff between Revisions #6 Feb 20, 2018

codenio mentioned this pull request Mar 1, 2018

Download remote chart to Archive dir #30

Merged

codenio mentioned this pull request Apr 5, 2018

Diff between Revisions #6

Closed

jeff-knurek mentioned this pull request Apr 12, 2018

FLAG: allow option for chart version #39

Closed

codenio mentioned this pull request Apr 23, 2018

Feature Request: Diff to compare rollback #24

Closed

databus23 requested changes Apr 25, 2018

View reviewed changes

databus23 reviewed Apr 25, 2018

View reviewed changes

databus23 reviewed Apr 26, 2018

View reviewed changes

databus23 approved these changes Apr 26, 2018

View reviewed changes

codenio closed this Apr 26, 2018

codenio deleted the feature/revision-diff branch April 26, 2018 14:41

codenio reopened this Apr 26, 2018

codenio force-pushed the feature/revision-diff branch from 4b0ee6b to 81d477f Compare April 26, 2018 15:38

codenio force-pushed the feature/revision-diff branch from 2b8f694 to 81d477f Compare April 26, 2018 16:11

codenio force-pushed the feature/revision-diff branch from a5cca6a to 81d477f Compare April 26, 2018 17:07

adding revision and rollback subcommands

72467ef

codenio force-pushed the feature/revision-diff branch from 81d477f to 72467ef Compare April 26, 2018 17:27

databus23 merged commit 9beb907 into databus23:master Apr 26, 2018

codenio mentioned this pull request Apr 27, 2018

Readme and Usage Updation : following #22 #49

Merged

databus23 added a commit that referenced this pull request Apr 27, 2018

Merge pull request #49 from aananthraj/feature/revision-diff

242ab78

Readme and Usage Updation : following #22

feature Implementation: Diff between Revisions #6 #22

feature Implementation: Diff between Revisions #6 #22

Conversation

codenio commented Jan 24, 2018

codenio commented Jan 26, 2018

databus23 commented Jan 26, 2018

codenio commented Jan 26, 2018

databus23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstarcher commented Apr 16, 2018

sfrique commented Apr 16, 2018

codenio commented Apr 20, 2018

codenio commented Apr 20, 2018

codenio commented Apr 23, 2018 • edited

codenio commented Apr 25, 2018 • edited

databus23 left a comment

Choose a reason for hiding this comment

databus23 Apr 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenio Apr 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenio Apr 25, 2018 • edited

Choose a reason for hiding this comment

codenio Apr 26, 2018 • edited

Choose a reason for hiding this comment

databus23 Apr 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

databus23 Apr 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

databus23 Apr 26, 2018 • edited

Choose a reason for hiding this comment

databus23 commented Apr 26, 2018 • edited

codenio commented Apr 26, 2018 • edited

sstarcher commented Apr 26, 2018

databus23 commented Apr 26, 2018

databus23 commented Apr 26, 2018 • edited

codenio commented Apr 26, 2018

databus23 commented Apr 26, 2018

databus23 commented Apr 26, 2018

codenio commented Apr 26, 2018 • edited

databus23 commented Apr 26, 2018

databus23 commented Apr 26, 2018

codenio commented Apr 27, 2018

sfrique commented Apr 27, 2018

codenio commented Apr 23, 2018 •

edited

codenio commented Apr 25, 2018 •

edited

databus23 Apr 25, 2018 •

edited

codenio Apr 25, 2018 •

edited

codenio Apr 25, 2018 •

edited

codenio Apr 26, 2018 •

edited

databus23 Apr 26, 2018 •

edited

databus23 Apr 26, 2018 •

edited

databus23 Apr 26, 2018 •

edited

databus23 commented Apr 26, 2018 •

edited

codenio commented Apr 26, 2018 •

edited

databus23 commented Apr 26, 2018 •

edited

codenio commented Apr 26, 2018 •

edited