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

Don't require nodePort to template if none is specified #606

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

jessebot
Copy link
Collaborator

@jessebot jessebot commented Jul 26, 2024

Description of the change

The actual port that the NodePort type service uses can be automatically generated if no nodeport field is specified, as per the docs here:
https://kubernetes.io/docs/concepts/services-networking/service/#type-nodeport

Benefits

We now use a with instead of an if to make the code a bit cleaner, instead of setting the nodePort to "" which I think may also be confusing and/or not work.

Possible drawbacks

none that I can think of, but always open to feedback :)

Applicable issues

Additional information

The actual value of nil in values.yaml has to change, otherwise, it interprets the nil as a string.

Checklist

@jessebot jessebot added the 3. to review Waiting for reviews label Jul 26, 2024
@jessebot
Copy link
Collaborator Author

jessebot commented Jul 26, 2024

oh no, why are all these tests failing? 🤔 Looks like it's because of this error:

Service in version "v1" cannot be handled as a Service: json: cannot unmarshal string into Go struct field ServicePort.spec.ports.nodePort of type int32

Let me see if I can fix it locally...

@jessebot
Copy link
Collaborator Author

ok, the fix was remove the nil from the values.yaml for the nodePort helm parameter 👍

@jessebot jessebot self-assigned this Jul 26, 2024
@jessebot jessebot removed the 3. to review Waiting for reviews label Jul 28, 2024
@jessebot jessebot changed the title Don't require nodePort or template at all if none is specified Don't require nodePort to template if none is specified Jul 29, 2024
@jessebot jessebot force-pushed the no-node-port branch 2 times, most recently from e2f7672 to 2e7099a Compare July 29, 2024 15:24
fixes nextcloud#44

Signed-off-by: jessebot <jessebot@linux.com>
Signed-off-by: WrenIX <dev.github@wrenix.eu>
Copy link
Collaborator

@wrenix wrenix left a comment

Choose a reason for hiding this comment

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

I rebase and set new chart version

@wrenix wrenix merged commit fae58fe into nextcloud:main Sep 12, 2024
9 checks passed
@jessebot jessebot deleted the no-node-port branch September 18, 2024 11:14
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.

unable to use nodePort service without setting a nodePort
3 participants