-
Notifications
You must be signed in to change notification settings - Fork 9
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
Remove grace period #19
base: master
Are you sure you want to change the base?
Conversation
this looks like it may have gotten tangled with the other PR. (this one's doing clusterip stuff as well.) maybe we only need to merge the one of these? it's nice and small regardless. |
{{- end }} | ||
# {{- if .Values.deployment.terminationGracePeriodSeconds }} | ||
# terminationGracePeriodSeconds: {{ .Values.deployment.terminationGracePeriodSeconds }} | ||
# {{- end }} |
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.
please delete unused code rather than commenting it out (unless it's in 'values.yml' where it can serve as a sample for how to configure things)
@@ -4,8 +4,8 @@ | |||
|
|||
iq: | |||
name: nxiq | |||
imageName: registry.connect.redhat.com/sonatype/nexus-iq-server | |||
imageTag: 1.85.0-01-ubi | |||
imageName: sonatype/nexus-iq-server |
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.
I think this should remain as RH
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.
could you elaborate on why the RH one is better? the docker.io image is lower-friction to install, since you don't need to do the whole authentication/key thing that red hat requires.
I'm comfortable needing to swap the image over on the certified operator side.
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.
If we do switch it, it would be good to reflect that in the readme, since i think there were specific instructions around defaults (RH) and alternate (docker.io) images.
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.
If you check the email from Neil Hartman on Jan 28th, he wrote:
While continuing our efforts to build a simple helm operator based off the stable helm chart I came across some issues. The main image the helm chart is pulling is the docker image for nexus, not your certified container image that you have hosted in the RHCC, therefore you would need to change that. There are also a few pieces (proxy, and backup) that call images that are hosted on quay. So if you were to certify this operator as is you would need to certify those proxy and backup images as well and reference those certified images in place of what is already being called.
There were a few back & forth comments later, but my understanding is that using the RHCC image is a requirement of certification.
{{- end }} | ||
# {{- if .Values.deployment.terminationGracePeriodSeconds }} | ||
# terminationGracePeriodSeconds: {{ .Values.deployment.terminationGracePeriodSeconds }} | ||
# {{- end }} |
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.
please delete rather than comment out the code
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 was this being deleted? It's gated, so if you don't want this value then don't provide it in your configuration. We can change the default if it's a problem.
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.
to @adrianpowell 's point, maybe this code can be left, and the entry in values.yaml can remain commented away? I think that was enough for my purposes over in the operator.
@@ -9,8 +9,8 @@ deploymentStrategy: {} | |||
# type: RollingUpdate | |||
|
|||
nexus: | |||
imageName: registry.connect.redhat.com/sonatype/nexus-repository-manager | |||
imageTag: 3.21.1-01-ubi-23 | |||
imageName: sonatype/nexus3 |
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 remain as RH
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.
I didn't have any issues with terminationGracePeriod
and and have found docs on this dating back quite a while. Was this removed recently? Before deleting it, I'd like to understand why it was a problem for you and not me & others
Issue #18
this clears out the error when deploying to minikube 1.10.1