-
Notifications
You must be signed in to change notification settings - Fork 51
🐛 Fix namespace conditionality, upgrade test #592
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
🐛 Fix namespace conditionality, upgrade test #592
Conversation
Signed-off-by: Mike Spreitzer <[email protected]>
1286b4d to
5ca0c46
Compare
|
|
||
| # Wait for deployment rollout to complete and all pods to be ready | ||
| echo "9. Waiting for deployment rollout to complete..." | ||
| if ! kubectl rollout status deployment/kubeflex-controller-manager -n kubeflex-system --timeout=300s; then |
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 same wait as is done in step 6; no need to repeat it.
0768486 to
87b25a0
Compare
|
/cc @francostellari |
Signed-off-by: Mike Spreitzer <[email protected]>
87b25a0 to
e2bbabb
Compare
| {{ template "NS" -}} | ||
| {{ end -}} | ||
| {{ end -}} | ||
| {{ else -}} |
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.
Are you using a with/else/end statement?
I was not aware that it was supported
Something does not seem to make sense to me
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.
Yes, that is one of the forms defined in https://pkg.go.dev/text/[email protected]#hdr-Actions .
The thing that that doc does not quite say clearly enough for my taste is that the value of an assignment is the value on the RHS of the assignment. But I do think that this is true, and is why the initial helm upgrade --install creates the namespace.
|
/cc @francostellari |
|
I tested these 4 configurations and they seem to work as expected: 1: helm upgrade --install kubeflex-operator kubeflex-ks/chart/ \
--set domain=localtest.me \
--set externalPort=94432 kubectl create ns kubeflex-system
helm upgrade --install kubeflex-operator kubeflex-ks/chart/ \
--set domain=localtest.me \
--set externalPort=94433 kubectl create ns kubeflex-system
helm upgrade --install kubeflex-operator kubeflex-ks/chart/ \
--namespace kubeflex-system \
--set domain=localtest.me \
--set externalPort=94434 helm upgrade --install kubeflex-operator kubeflex-ks/chart/ \
--namespace kubeflex-system --create-namespace \
--set domain=localtest.me \
--set externalPort=9443 |
|
however, for completeness, if an upgrade command specifies a different namespace of the previous helm command it will fail helm upgrade --install kubeflex-operator kubeflex-ks/chart/ \
--namespace kubeflex-system --create-namespace \
--set domain=localtest.me \
--set externalPort=9443then helm upgrade --install kubeflex-operator kubeflex-ks/chart/ \
--set domain=localtest.me \
--set externalPort=9443fails: I'm not sure how this could be fixed at the moment and it's probably outside of this scope. |
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 370c9b0587a9956dcbf8f2b72e4513d5bf1269e7 |
@MikeSpreitzer with these review I think we can move forward to the next steps |
|
I think that #592 (comment) is outside the usage envelope that we intend to support. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MikeSpreitzer The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary
This PR updates the namespace template in the Helm chart to properly handle a second
helm upgrade, following an outline from @francostellari. This PR also makes the E2E test case for this executable and adds it to the set of test cases that actually run. This PR also fixes the waits and the display of Pods in that test case.Related issue(s)
Fixes #453