-
Notifications
You must be signed in to change notification settings - Fork 671
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
Add backward compatibility checks for api changes #2114
Conversation
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
35450d6
to
d3396a3
Compare
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.
have some questions though, i've been worried about this change for a while. how will this affect existing CRDs on a cluster?
@@ -24,7 +24,10 @@ jobs: | |||
end-to-end-kustomize: | |||
runs-on: ubuntu-latest | |||
steps: | |||
- uses: actions/checkout@v1 | |||
- uses: actions/checkout@v2 | |||
- uses: actions/setup-go@v2 |
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 did we not need go before?
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 in my attempt to keep the environment consistent for make helm
in CI and locally until we introduce a docker image to build helm template :-))))
@@ -21,4 +26,7 @@ spec: | |||
type: object | |||
x-kubernetes-preserve-unknown-fields: true | |||
properties: | |||
{{- else }} | |||
version: v1alpha1 |
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 this be plural? or the one above singular?
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 the deprecated form.. you could only have one... the new api allows multiple versions declared in the CRD...
script/generate_helm.sh
Outdated
@@ -38,7 +39,7 @@ if [ -n "$DELTA_CHECK" ]; then | |||
echo "diff detected: $DIFF" | |||
DIFF=$(git diff --name-only) | |||
echo "files different: $DIFF" | |||
exit 1 | |||
exit 0 |
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.
so always succeed?
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.
Ops... nooo... that was me playing with the script... thanks for catching this!
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
As described here. The work done to move to the latest K8s APIs would break backward compatibility with older clusters. This PR adds compatibility check to use the corresponding K8s APIs
Signed-off-by: Haytham Abuelfutuh [email protected]