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

update mini notebook image and modify targetport #226

Merged
merged 2 commits into from
Nov 18, 2024
Merged

Conversation

PaulineMauryL
Copy link
Member

No description provided.

@PaulineMauryL PaulineMauryL force-pushed the wip_371_k8s branch 2 times, most recently from b2f2dfc to 3b8faae Compare November 15, 2024 11:10
@PaulineMauryL
Copy link
Member Author

Coverage report

This PR does not seem to contain any modification to coverable code.

@@ -8,7 +8,7 @@ spec:
type: {{ .Values.server.service.type }}
ports:
- port: {{ .Values.server.service.port}}
targetPort: http
Copy link
Member Author

Choose a reason for hiding this comment

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

i wonder why this works ?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah ok, the names 'http' match and then everything is 80, maybe it is better to differentiate the service port as 80 and container port as 8080

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, we could allow to specify the target port in the values file too.

@PaulineMauryL PaulineMauryL marked this pull request as ready for review November 15, 2024 13:01
@PaulineMauryL
Copy link
Member Author

not tested yet

@@ -30,7 +30,7 @@ spec:
imagePullPolicy: {{ .Values.server.image.pullPolicy }}
ports:
- name: http
containerPort: {{ .Values.server.service.port }}
containerPort: {{ .Values.server.runtime_args.settings.server.host_port }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am trying to wrap my head around this one. Did it work because service.port was set to 80 and host port was http in the templates so since everything was 80 it so happened that it was ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, i think it was very lucky because everything was 80

@PaulineMauryL PaulineMauryL merged commit 5c8aaf3 into develop Nov 18, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants