USHIFT-673: Clean up config file format #1187
Conversation
openshift-ci
bot
added
the
approved
fzdarsky
force-pushed
the
structure-config
branch
from
ae22489
to
a0caa3e
Compare
fzdarsky
force-pushed
the
structure-config
branch
from
a0caa3e
to
8b282a2
Compare
|
/approve |
|
For me this structure makes a lot of sense. I believe it would be a good idea to go with this structure for 4.12 |
|
hostnameOverride: "" is a better name than what we had before because it's more specific about the intent, and seems ok to leave it empty, while before it was confusing if you had to provide it. |
There was a problem hiding this comment.
This changes some of the fields I asked @dgrisonnet and @benluddy to include in their enhancement, so I'd like them to look at the PR.
I had a couple of questions inline, too.
openshift-merge-robot
added
the
needs-rebase
fzdarsky
force-pushed
the
structure-config
branch
from
8b282a2
to
1f5bb21
Compare
openshift-merge-robot
removed
the
needs-rebase
| baseDomain: "" | ||
| node: | ||
| hostnameOverride: "" | ||
| nodeIP: "" |
There was a problem hiding this comment.
This field is (currently) used for many things that are unrelated to Node/kubelet. Unless that's going to change, I think putting it into a "node" config group is misleading.
There was a problem hiding this comment.
I can see your point, though I wouldn't call those "many things" unrelated:
We currently use nodeName for a) TLS certs for services on the node, b) in etcd config making the node member, c) as name for the node in kubelet, d) in mDNS to announce the node. In all cases but kubelet, that node name will be a domain name that resolves to the same IP address. In the case of kubelet, the node name defaults to the host name, thus also pointing to the same IP address. It may be overridden (with --override-hostname) to something else, of course, but would still need to be resolvable.
Is there another grouping that'd make more sense to you? Should I revert to "nodeName" (although that also suggests "node") or use hostnameOverride (although we use it for things unrelated to kubelet)?
| @@ -67,7 +67,7 @@ func TestConfigure(t *testing.T) { | |||
| "--secure-port=10257", | |||
| fmt.Sprintf("--service-account-private-key-file=%s", kcmServiceAccountPrivateKeyFile()), | |||
| "--use-service-account-credentials=true", | |||
| "-v=0", | |||
| "-v=2", | |||
There was a problem hiding this comment.
we should set it to Normal to make sure that the new UX works
There was a problem hiding this comment.
"Normal" corresponds to "2", correct?
There was a problem hiding this comment.
@dgrisonnet we commented about that here. Do you mean that?
Signed-off-by: Frank A. Zdarsky <fzdarsky@redhat.com>
Signed-off-by: Frank A. Zdarsky <fzdarsky@redhat.com>
Signed-off-by: Frank A. Zdarsky <fzdarsky@redhat.com>
Signed-off-by: Frank A. Zdarsky <fzdarsky@redhat.com>
fzdarsky
force-pushed
the
structure-config
branch
from
1f5bb21
to
9885544
Compare
mangelajo
requested review from
dhellmann,
benluddy,
dinhxuanvu and
dgrisonnet
|
/retest |
|
@fzdarsky: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
I'll take the shot of tagging this. I think there were great points and discussions on the PR. Some of them have been addressed and I think the result makes a lot of sense. If there is a need to make a minor change, it could be done in a follow PR, but let's get this merged since it's important for 4.12. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fzdarsky, mangelajo, oglok The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Closes USHIFT-673