-
Notifications
You must be signed in to change notification settings - Fork 4.8k
add serializeable start config #1247
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
add serializeable start config #1247
Conversation
ecc5f22 to
1bffe4f
Compare
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.
not sure what the behavior of user.GetName() is if UserFrom says it didn't exist
27e0546 to
d860c65
Compare
|
@liggitt don't freak out. I just had to burn the village to save it. /server is dead, long live /startconfig |
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.
don't make this look like a real username... make it "Unknown user" or something
418bb26 to
202ec27
Compare
|
Alright, I'm happy with the shape of this now. It allows you to separately mint certs. All in one launch delegates down to master and node like you would expect. It can generate the fully specified for configs for you and read them back in. I need to fix up unit tests and integration tests, but I think I like the final shape of things. Next steps for future pulls: There was more to that list when I started it..... |
3c2e61f to
6084f8e
Compare
|
List of things to make sure don't get dropped:
Suggestions for future pulls:
|
pkg/cmd/server/start/node.go
Outdated
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.
It's super confusing for kubernetes.NodeConfig.NodeHost to not come from configapi.NodeConfig.NodeHost. I actually prefer NodeID, but can we name the other one BindHost?
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.
Done. NodeID versus Hostname is going to an upstream back down to us thing. Otherwise it's always confusing in some location or another.
c2c4572 to
3f6741a
Compare
|
rebased |
|
Seems like this should not complain about DNS, and should tell you a master and node config path are required: |
pkg/cmd/server/api/v1/types.go
Outdated
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.
lowercase leading
1328d29 to
92efff0
Compare
|
rebased |
pkg/cmd/server/api/helpers.go
Outdated
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.
this won't resolve file references relative to the filepath :-/
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.
We'll get that working with kubernetes/kubernetes#5541. For now, the generated .kubeconfig files are self contained.
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.
Using the upstream patch for now.
|
all-in-one isn't setting ClusterDNS on node args to the master IP |
|
Opened deads2k#6 to improve DNS handling:
@smarterclayton can you eyeball the DNS changes in my pull? |
e9a1f1d to
830ad88
Compare
|
relative file references in the master/node config files get resolved relative to |
db754a4 to
6ef4884
Compare
6ef4884 to
3038033
Compare
|
LGTM |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1196/) (Image: devenv-fedora_1061) |
|
Evaluated for origin up to 3038033 |
Merged by openshift-bot
More unit tests need to be added, but you can start to see the bones.
Before merging:
Next list:
@liggitt