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.

Sign up for GitHub

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

switching to clusterIP #15

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

switching to clusterIP #15

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b6f8a3d
switching to clusterIP
CMYanko May 26, 2020
cc642ba
stock image current version
CMYanko May 26, 2020
a979c55
nxrm to stock image and lv3.23.0
CMYanko May 26, 2020
111ed71
Moved Service Type to values.txt
CMYanko May 27, 2020
c0bf50e
removed unused code, added adminPort
CMYanko Jun 4, 2020
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
View file
Open in desktop
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
View file
Open in desktop
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.

interesting. i had also found this setting to be problematic in the operator code, and had to remove reference to it in the values.

Copy link
Contributor

Choose a reason for hiding this comment

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

if this isn't useful, then please delete it. Commented out code creates a lot of clutter

Copy link
Contributor

Choose a reason for hiding this comment

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

@jflinchbaugh why was this a problem? I believe I added it in because of some specific concerns that the shutdown would not be done in time (which was something I experienced personally). It is optional so I don't see the value in removing this support.

Copy link
Author

Choose a reason for hiding this comment

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

It is useful on OCP, unsupported on k8s

lifecycle:
{{- if .Values.deployment.postStart.command }}
postStart:
Expand Down
2 changes: 1 addition & 1 deletion OpenShift/helm/nexus-iq/templates/service.yaml
View file
Open in desktop
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: {{ .Values.iq.serviceType }}
6 changes: 4 additions & 2 deletions OpenShift/helm/nexus-iq/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@

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.

we had used the RH version because that was a requirement for getting this chart certified. Why are you changing this? It's in the values file so if you need to override it, you can but I think we should stick with the RH as a default.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed in the context of an Operator but we chose to use helm so we also get helm charts of this for generic k8s instances

imageTag: 1.91.0
Copy link
Contributor

Choose a reason for hiding this comment

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

if we're going to upgrade this now, how do we feel about combining the version into imageName? The operator requires it to be combined, but does it go against a rival best-practice for helm?

Copy link
Contributor

Choose a reason for hiding this comment

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

we had kept them separate because the build process would try to hunt out the version and update it automatically. It may be possible to combine it, but we'd need to update the Jenkinsfile and it will likely introduce some more complexity there. Definitely a trade-off.

Copy link
Contributor

@jflinchbaugh jflinchbaugh May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I probably shouldn't have tried to lump that combination into this PR, so you're right, let's not try to combine name and tag just yet. ignore my suggestion.

Copy link
Author

Choose a reason for hiding this comment

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

For helm, the repo, imagename, and tags should all be separate entries so they can be ovrridden

imagePullPolicy: IfNotPresent
imagePullSecret: ""
applicationPort: 8070
adminPort: 8071
memory: 1Gi
service:
type: ClusterIP
# base 64 encoded license file with no line breaks
licenseSecret: ""
# configYaml is the full text of the config.yml file that will be passed to IQ Server
Expand Down
2 changes: 1 addition & 1 deletion OpenShift/helm/nexus-repository-manager/Chart.yaml
View file
Open in desktop
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
6 changes: 3 additions & 3 deletions OpenShift/helm/nexus-repository-manager/values.yaml
View file
Open in desktop
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.

same concern as earlier - I believe this should remain using RH as the default image source

Copy link
Author

Choose a reason for hiding this comment

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

same reply, we should use our stock images here in a helm chart and John can switch to the RHCC for the operator

imageTag: 3.23.0
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the upgrade. as in the other values.yaml, could the version be pushed into imageName?

Copy link
Contributor

Choose a reason for hiding this comment

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

🙅 my suggestion is invalid at this point. this should not be combined now.

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