Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion OpenShift/helm/nexus-iq/Chart.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ apiVersion: v1
appVersion: 1.84.0
description: A Helm chart for Kubernetes
name: iqserver
version: 0.1.0
version: 0.1.1
description: Sonatype Nexus IQ Server continuously monitors your entire software supply chain
keywords:
- sonatype
Expand Down
6 changes: 3 additions & 3 deletions OpenShift/helm/nexus-iq/templates/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ spec:
image: {{ .Values.iq.imageName }}:{{ .Values.iq.imageTag }}
ports:
- containerPort: {{ .Values.iq.applicationPort }}
{{- if .Values.deployment.terminationGracePeriodSeconds }}
terminationGracePeriodSeconds: {{ .Values.deployment.terminationGracePeriodSeconds }}
{{- end }}
# {{- if .Values.deployment.terminationGracePeriodSeconds }}
# terminationGracePeriodSeconds: {{ .Values.deployment.terminationGracePeriodSeconds }}
# {{- end }}
Copy link
Contributor

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)

lifecycle:
{{- if .Values.deployment.postStart.command }}
postStart:
Expand Down
2 changes: 1 addition & 1 deletion OpenShift/helm/nexus-iq/templates/service.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ spec:
port: {{ .Values.iq.applicationPort }}
- name: admin
port: {{ .Values.iq.adminPort }}
type: NodePort
type: ClusterIP
4 changes: 2 additions & 2 deletions OpenShift/helm/nexus-iq/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

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

Copy link
Contributor

@jflinchbaugh jflinchbaugh May 28, 2020

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.

Copy link
Contributor

@jflinchbaugh jflinchbaugh May 28, 2020

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.

Copy link
Contributor

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.

imageTag: 1.91.0
imagePullPolicy: IfNotPresent
imagePullSecret: ""
applicationPort: 8070
Expand Down
2 changes: 1 addition & 1 deletion OpenShift/helm/nexus-repository-manager/Chart.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
apiVersion: v1
name: sonatype-nexus
version: 0.2.0
version: 0.2.1
appVersion: 3.20.1-01
description: Sonatype Nexus is an open source repository manager
keywords:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ spec:
- name: nexus
image: {{ .Values.nexus.imageName }}:{{ .Values.nexus.imageTag }}
imagePullPolicy: {{ .Values.nexus.imagePullPolicy }}
{{- if .Values.deployment.terminationGracePeriodSeconds }}
terminationGracePeriodSeconds: {{ .Values.deployment.terminationGracePeriodSeconds }}
{{- end }}
# {{- if .Values.deployment.terminationGracePeriodSeconds }}
# terminationGracePeriodSeconds: {{ .Values.deployment.terminationGracePeriodSeconds }}
# {{- end }}
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

@jflinchbaugh jflinchbaugh May 28, 2020

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.

lifecycle:
{{- if .Values.deployment.postStart.command }}
postStart:
Expand Down
6 changes: 3 additions & 3 deletions OpenShift/helm/nexus-repository-manager/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should remain as RH

imageTag: 3.23.0
imagePullPolicy: IfNotPresent
imagePullSecret: ""
env:
Expand All @@ -36,7 +36,7 @@ nexus:
dockerPort: 5003
nexusPort: 8081
service:
type: NodePort
type: ClusterIP
# clusterIP: None
# annotations: {}
## When using LoadBalancer service type, use the following AWS certificate from ACM
Expand Down