-
-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
kubernetes: fix missing port if advertiseAddress specified #99226
base: master
Are you sure you want to change the base?
Conversation
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Result of |
I marked this as stale due to inactivity. → More info |
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.
Here’s how I was able to test out this change.
- Create a file named
kubernetes-port-test.nix
- Put the following into that file:
# 🅭🄍1.0 This file is dedicated to the public domain using the CC0 1.0 Universal # Public Domain Dedication: # <https://creativecommons.org/publicdomain/zero/1.0/> let configuration = { services.kubernetes.apiserver = { enable = true; advertiseAddress = "example.com"; securePort = 1234; }; }; pkgs = import path/to/nixpkgs { }; nixOSPackage = pkgs.nixos configuration; in nixOSPackage.config.services.kubernetes.apiserverAddress
- Run
nix-instantiate --eval kubernetes-port-test.nix
When I do that procedure with Nixpkgs’s master
branch checked out, nix-instantiate
prints "https://example.com"
. When I do that preocedure with this PR’s branch checked out, nix-instantiate
prints "https://example.com:1234"
. In other words, I think that this PR works properly.
I also tried running the tests for the Kubernetes module. nix-build -A nixosTests.kubernetes.dns
failed on my system, but that test also fails when I run it with this PR’s parent commit, so I don’t think that this PR needs to address that failure. nix-build -A nixosTests.kubernetes.rbac
succeeded on my system.
Here’s what I think should be changed:
- The commit message should start with “nixos/kubernetes:” (See this section of
nixos/README.md
.) - I would take the “Motivation for this change” section of the PR comment, and put it in the body of the commit message. That will make it easier for people to understand this change when using
git-log
.
Motivation for this change
If
apiserver.advertiseAddress
andapiserver.securePort
were specified, the wrong port was used inside the default value ofapiserverAddress
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)