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

Test chart upgrades #103

Merged
merged 8 commits into from
Mar 18, 2019
Merged

Conversation

jlegrone
Copy link
Member

@jlegrone jlegrone commented Feb 12, 2019

What this PR does / why we need it:
This PR enables testing in-place upgrades of charts that have non-breaking changes according to the semver spec.

The desired behavior of this PR and further discussion can be found here: #25 (comment)

Which issue this PR fixes

closes #25
closes #19
closes #99
closes #100

Special notes for your reviewer:

Some TODOs:

  • Handle uncommitted changes in git repositories
  • Handle not being in a git repository
  • Add a config flag to opt out of upgrade testing
  • Consider the need for more tests (PR Add Helm/Kubernetes integration tests #120)
  • Change the Git interface to a "SCM" interface to avoid further tight coupling with git? (out of scope for this PR)

Known caveats:

  1. Only works with git
  2. If a chart has direct dependencies outside of its parent directory, helm dependency build may fail or not produce the same result as the previous revision

Run integration tests with go test -cover -tags=integration ./... (see #120)

@helm-bot helm-bot added size/L and removed size/L labels Feb 12, 2019
@helm-bot helm-bot added size/L and removed size/L labels Feb 13, 2019
@helm-bot helm-bot added size/L and removed size/L labels Feb 13, 2019
@helm-bot helm-bot added size/L and removed size/L labels Feb 13, 2019
@helm-bot helm-bot added size/L and removed size/L labels Feb 14, 2019
@jlegrone jlegrone changed the title [WIP] Test chart upgrades Test chart upgrades Feb 14, 2019
@jlegrone
Copy link
Member Author

@unguiculus I'd appreciate a first pass when you have a chance!

@unguiculus
Copy link
Member

Thanks. Please give me some time to review and test this. From quickly skimming over it: Have you thought about installing the previous version from the repo instead of checking out the merge base? Doing so, you assume the merge base reflects the previous release. This doesn't necessarily have to be the case.

@jlegrone
Copy link
Member Author

I did consider pulling the previous published revision, but I think there are a couple problems with that:

  1. We don't necessarily know where the charts have been published -- I suppose we could handle this with a new configuration flag.
  2. Not all teams publish their charts to a registry. It is a common use case to version a chart alongside your application source code and run helm commands using the local chart path.

@helm-bot helm-bot removed the size/L label Feb 17, 2019
Signed-off-by: Jacob LeGrone <git@jacob.work>
Signed-off-by: Jacob LeGrone <git@jacob.work>
Signed-off-by: Jacob LeGrone <git@jacob.work>
@helm-bot helm-bot added size/L and removed size/L labels Mar 11, 2019
@helm-bot helm-bot added size/L and removed size/L labels Mar 11, 2019
pkg/chart/chart.go Outdated Show resolved Hide resolved
@helm-bot helm-bot added size/L and removed size/L labels Mar 12, 2019
Co-Authored-By: jlegrone <jlegrone@users.noreply.github.com>
Signed-off-by: Jacob LeGrone <git@jacob.work>
Signed-off-by: Jacob LeGrone <git@jacob.work>
@unguiculus
Copy link
Member

LGTM but @scottrigby should also approve.

Copy link
Member

@scottrigby scottrigby left a comment

Choose a reason for hiding this comment

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

@jlegrone @unguiculus I buried my comment in a resolved thread #103 (comment) - this looks good to me 👍

@scottrigby scottrigby merged commit de59b1c into helm:master Mar 18, 2019
@jlegrone jlegrone deleted the feature/upgrade-testing branch March 18, 2019 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants