Skip to content

Conversation

@zytek
Copy link

@zytek zytek commented May 23, 2018

Just took #29 and updated it to work with master.

Copy link
Collaborator

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

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

@zytek @njam Thank you so much for this! The implementation LGTM.

Although this seems nice to have in helm-diff, it is the most complex logic in the code-base. That makes me wanting to add CI :)

So, would you mind adding a single test-case to printDiff for the case context is bigger than zero? So that I can setup CI to run it.

Please tell me if you don't have bandwidth to do so :)


revisionCmd.Flags().BoolP("suppress-secrets", "q", false, "suppress secrets in the output")
revisionCmd.Flags().StringArrayVar(&diff.suppressedKinds, "suppress", []string{}, "allows suppression of the values listed in the diff output")
revisionCmd.Flags().IntVarP(&diff.outputContext, "context", "C", -1, "output NUM lines of context around changes")
Copy link
Collaborator

Choose a reason for hiding this comment

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

For anyone interested - this corresponds to the equivalent option in the diff command:

$ diff --help | grep -- -C
  -c  -C NUM  --context[=NUM]  Output NUM (default 3) lines of copied context.

@njam
Copy link
Contributor

njam commented Jun 4, 2018

adding a single test-case to printDiff

I will gladly do it, to learn how it's done! I can do it on the next weekend. If somebody else wants to do it beforehand, feel free.
Just to be sure, you mean to add a unit test that calls printDiff() and checks the return value right?

UPDATE: It's done in #29

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 27, 2018

@zytek @njam I've finally reviewed and merged #29. So closing this in favor of that. Thanks for your patience!

@mumoshu mumoshu closed this Jun 27, 2018
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