-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Upgrade helm-operator to Helm v3 #2080
Conversation
go.mod
Outdated
github.com/operator-framework/operator-lifecycle-manager v0.0.0-20190128024246-5eb7ae5bdb7a | ||
|
||
// TODO: Can OLM use valid go modules version numbers? | ||
github.com/operator-framework/operator-lifecycle-manager v0.0.0-20190926160646-a61144936680 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've been wanting to, but some openshift automation added a v3.11
tag to our repository, which would force us to start at a greater semver for minimal version selection to skip it. IIRC, this happened right as we started to use modules (go 1.11 timeframe). We should revisit this soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no! That's a bummer. I don't envy you. However, since there's no go.mod
file on the commit tagged with v3.11.0
, you might not be totally hosed. See https://github.com/golang/go/wiki/Modules#semantic-import-versioning, specifically item 2 in the "Three Transitional Exceptions" section.
It might be worth trying to add a valid go module semver tag in a throwaway fork at v0.12.0
to see whether you can import at that version without getting bumped "up" to v3.11.0
. I'm pretty sure that will work.
Also since v3.11.0
is not a go module (since it is lacking a go.mod
file), you may even be able to delete the v3.11.0
tag with little consequences. Don't quote me on that, but that's something else that might be worth trying in a throwaway fork.
691e715
to
29a3410
Compare
de944e7
to
3f7eed8
Compare
@joelanford: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
pkg/helm/release/manager_factory.go
Outdated
return releaseHistory, len(releaseHistory) > 0, nil | ||
} | ||
|
||
func releaseHistoryV3(storageBackend *v3storage.Storage, releaseName string) ([]*helmreleasev3.Release, bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is required have releaseHistoryV2 and releaseHistoryV3? Will we keep the compatibility with old versions?
Should it still work with v2 helms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary to handle automatically migrating existing Helm v2 releases to Helm v3. The idea here is that a user with existing CRs and an existing Helm v2-based operator can upgrade to the new Helm 3-based operator and the new operator will handle the transition seamlessly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It mainly shows fine. I added a few questions/comments.
Also, it is missing the CHANGELOG and update all link docs to address the users check the refs in the helm3 docs instead of 2.
internal/scaffold/helm/role.go
Outdated
return nil, fmt.Errorf("failed to render chart templates: %s", err) | ||
} | ||
return tiller.SortByKind(releaseutil.SplitManifests(renderedTemplates)), nil | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not it be removed? Why keep this code commented?
it could be my environment, but I built sdk for this PR, ran the user guide steps and am the CR shows size of 2....
kind: List but only a single pod and deployment is running.... I'm seeing this in the operator log: here is my test env: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, just a bunch of nits and questions.
seems to test out ok on OCP 4.2, I deployed using the master branch (helm v2), then deleted the nginx-operator, then built a new nginx-operator against this PR code, redeployed the nginx-operator, then updated the CR to add a new nginx pod, seems to work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
New changes are detected. LGTM label has been removed. |
Description of the change:
This PR bumps Helm to v3.0.0, adds the helm-2to3 plugin to auto migrate existing CRs, and refactors relevant portions of the helm operator code due to the restructuring between helm 2 and helm 3
Motivation for the change:
To support Helm 3 in Operator SDK