-
Notifications
You must be signed in to change notification settings - Fork 57
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
Fixing the helm chart integration tests. #75
Fixing the helm chart integration tests. #75
Conversation
67d4540
to
5766de9
Compare
/assign |
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 we be creating a new package instead of overwriting the existing 0.4.0? I don't know what is considered best practice here for releasing templates
5766de9
to
4b0f2fb
Compare
@marosset @jsturtevant I addressed all of your feedback. Thanks for the reviews. |
4b0f2fb
to
9e538f2
Compare
9e538f2
to
e8a0c50
Compare
Looks like we now have two folders for charts. I've done some research and it looks like don't need to version the folder that the chart lives in itself which is why we are having a bit of trouble here. Looking at https://v2.helm.sh/docs/developing_charts/#store-charts-in-your-chart-repository (I found this example as well: https://github.com/kubernetes-sigs/cloud-provider-azure/tree/master/helm) We can have a directory structure like:
The workflow would be make updates to the The tests themselves would reference the Does that make sense? |
we could fix the test here, then open a separate PR to move these files around to the correct location which might make more sense. |
e8a0c50
to
18d01c0
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.
lgtm, Thanks for cleaning all this up. Do we need to update the readme install instructions with the correct url to add the repository?
Test is using the helm chart 🚀
installing helm deployment windows-gmsa-dev with chart /home/runner/work/windows-gmsa/windows-gmsa/charts/gmsa and image :
KUBECONFIG=/home/runner/work/windows-gmsa/windows-gmsa/admission-webhook/dev/kubeconfig-windows-gmsa-chart-integration /usr/local/bin/kubectl create namespace windows-gmsa-dev
namespace/windows-gmsa-dev created
KUBECONFIG=/home/runner/work/windows-gmsa/windows-gmsa/admission-webhook/dev/kubeconfig-windows-gmsa-chart-integration /usr/local/bin/helm version
version.BuildInfo{Version:"v3.9.0", GitCommit:"7ceeda6c585217a19a1131663d8cd1f7d641b2a7", GitTreeState:"clean", GoVersion:"go1.17.5"}
KUBECONFIG=/home/runner/work/windows-gmsa/windows-gmsa/admission-webhook/dev/kubeconfig-windows-gmsa-chart-integration /usr/local/bin/helm install windows-gmsa-dev /home/runner/work/windows-gmsa/windows-gmsa/charts/gmsa --namespace windows-gmsa-dev
NAME: windows-gmsa-dev
LAST DEPLOYED: Wed May 25 16:13:50 2022
NAMESPACE: windows-gmsa-dev
STATUS: deployed
REVISION: 1
TEST SUITE: None
KUBECONFIG=/home/runner/work/windows-gmsa/windows-gmsa/admission-webhook/dev/kubeconfig-windows-gmsa-chart-integration /usr/local/bin/kubectl wait -n windows-gmsa-dev pod -l app=windows-gmsa-dev --for=condition=Ready
pod/windows-gmsa-dev-6c6b5ddffb-njff4 condition met
make[1]: Leaving directory '/home/runner/work/windows-gmsa/windows-gmsa/admission-webhook'
### Starting integration tests with Kubernetes version: 1.23.4 ###
18d01c0
to
cf7d1cd
Compare
This PR makes tweaks and fixes some assumptions that prevented the integration test from actually working. This also re-organizes the charts to a different structure.
cf7d1cd
to
b0f6b3a
Compare
/lgtm |
Do we need to update https://github.com/kubernetes-sigs/windows-gmsa/blob/master/RELEASE.md? |
@marosset I would say not yet, I think we need to automate the generation and update of the index. |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: marosset, phillipsj The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.