-
Notifications
You must be signed in to change notification settings - Fork 189
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
redhat-dotnet-imagestreams: add .NET 9.0 images, remove EOL .NET 7 images. #1653
base: main
Are you sure you want to change the base?
redhat-dotnet-imagestreams: add .NET 9.0 images, remove EOL .NET 7 images. #1653
Conversation
@omajid @aslicerh @komish ptal. Besides adding the 9.0 version and removing the 7.0 version, I've also refactored the chart so it uses templates to avoid the duplication. For future version updates we can then limit ourselves to changing the Chart.yaml and data.yaml files. The goal is to merge this at the 9.0 GA which will then make the new chart available to existing OpenShift installations. |
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.
Thanks, this is amazing!
charts/redhat/redhat/redhat-dotnet-imagestreams/0.0.2/src/templates/dotnet-imagestream.yaml
Outdated
Show resolved
Hide resolved
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.
The chart is currently failing chart testing when chart-verifier is running against it. I know we're only installing imagestreams here, but does it make sense to include a basic test that confirms the imagestream installed without issue?
I'm trying to figure out what looks like. I see many charts include a Do these tests rely on the presence of any test causing the image streams to get imported before running that test, and in that manner verify the imagestreams are importable? |
charts/redhat/redhat/redhat-dotnet-imagestreams/0.0.2/src/Chart.yaml
Outdated
Show resolved
Hide resolved
Yes. Traditional helm testing (e.g.
To my eye, this test looks invalid. I think a valid test might look something like this: apiVersion: v1
kind: Pod
metadata:
name: "{{ .Release.Name }}-imagestream-test"
namespace: "{{ .Release.Namespace }}"
annotations:
"helm.sh/hook": test
"alpha.image.policy.openshift.io/resolve-names": '*'
spec:
containers:
- name: dotnet-test
image: dotnet:8.0-ubi8
imagePullPolicy: IfNotPresent
command:
- '/bin/bash'
- '-ec'
- 'echo helloworld'
- name: dotnet-runtime
image: dotnet-runtime:8.0
imagePullPolicy: IfNotPresent
command:
- '/bin/bash'
- '-ec'
- 'echo helloworld'
restartPolicy: Never Notably, the The ability to install this chart in any namespace is critically important. I see your values.yaml includes a namespace definition, but it doesn't seem to be referenced anywhere. In our hosted pipeline, charts are installed to a fresh namespace, so if this hardcoded namespace 'openshift' were actually used, I'm sure this test would fail. Hope this helps. Let me know how else I can assist. |
This was also copied from other imagestream charts in the repo. Probably this issue applies to all of them. |
@komish I've added a test based on your example. I assume it is not running in CI currently? Can you trigger it to know how it behaves? Also: what should I do about the |
Easiest thing to do is install chart-testing and install your chat from a local repo. Requires some setup, but will be the fastest thing. Otherwise, just
I don't think you need this, but it's up to you and your chart's design. Ultimately, Helm provides All that to say - you have options here, and I'm not sure if namespace is necessary in your values.yaml. To that end, you should be okay to remove it so long as it makes sense to you. |
This is what I ended up doing.
I've removed it, and I think it would make sense for the other charts to do the same. The parameter implies the install would be in that namespace, but as you have pointed out: it uses the current context instead. The test is now checking all tags of the image streams and whether those include the right .NET versions for runtime/SDK. @komish thanks for all your help!! |
@tmds Some of your new tests are failing. Specifically, I see those for .NET 9* images and the "latest" image (presumably because version 9 is your latest tag) are hitting imagePullBackOff. This is what I get when I run
Here are the conditions on the ImageStreams for the v9* tags
|
.NET 9 images haven't been released yet. That will happen in the next week(s). The test should pass then, and then we can merge this. |
Whoops! Makes sense. Thanks! |
No description provided.