-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Create YAML or JSON Clusterspec without creating the cluster #2954
Create YAML or JSON Clusterspec without creating the cluster #2954
Conversation
/assign @andrewsykim @sethpollack @geojaz @justinsb We only need one, unless you are working on getting more PR reviews under your belt :) |
@chrislovecnm: GitHub didn't allow me to assign the following users: sethpollack. Note that only kubernetes members can be assigned. In response to this:
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. |
AsfFor me and said on slack :
I think it would confuse more people to have it in the Would vote for the |
b625dc5
to
504d9a4
Compare
In terms of naming this on slack, we had two that liked |
504d9a4
to
a486d28
Compare
cmd/kops/create_cluster.go
Outdated
switch targetName { | ||
case cloudup.TargetYAML: | ||
|
||
var clusters []*api.Cluster |
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.
Can I inline the creation of this var? Could not figure out the syntax ...
Marking WIP, as I need to refactor a bit, and figure out if we want to have init or create. |
a486d28
to
dd96bed
Compare
kubectl uses
Still not sure what we're going to do in kops server mode, where k8s doesn't really have a user-driven "apply changes" step (our |
So kubectl does have a dry run, but is this a dry run? Dry run to me means, execute something that will do something, and test that it will do the correct actions, without doing anything. That is pretty much what helm dry run does, but you have to have a manifest first. This is something that helm, kubectl, etc do not really have ... Maybe init ... pondering |
Let's hold off on non-critical UX changes until we are building the UX for kops-server; I don't want to paint ourselves into a corner. |
I like |
@justinsb this is the PR that I meant to comment on. This appears to be orthogonal to the kops-server. This is a utility that anyone would use to create yaml files to interact with the server. This is something that kubectl, helm, frankly any component in k8s, that I know of, does not have. Please exaplain how this would impact the kops-server. |
@justinsb PTAL - this was asked for on office hours |
dd96bed
to
285c7e8
Compare
cmd/kops/create_cluster.go
Outdated
} | ||
|
||
var targetOptions = sets.NewString(cloudup.TargetDryRun, cloudup.TargetCloudformation, cloudup.TargetDirect, cloudup.TargetTerraform) |
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.
I don't think dryrun should be an explicit target here in the CLI.
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.
correct ... will remove
cmd/kops/create_cluster.go
Outdated
case OutputYaml: | ||
|
||
var clusters []*api.Cluster | ||
cluster.ObjectMeta.CreationTimestamp = v1.NewTime(time.Now()) |
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 looks like kubectl create secret generic dummy --dry-run -oyaml
does not set CreationTimestamp.
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.
I can try w/o, but I do not think kops create -f mycluster.yaml
works w/o it.
cmd/kops/create_cluster.go
Outdated
switch c.Output { | ||
case OutputYaml: | ||
|
||
var clusters []*api.Cluster |
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.
Hoist the common code out of the switch I think
@@ -1008,6 +1065,7 @@ func RunCreateCluster(f *util.Factory, out io.Writer, c *CreateClusterOptions) e | |||
} | |||
} | |||
|
|||
// Can we acutally get to this if?? |
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.
Do you mean that targetName is always != ""
?
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.
Is targetName ever == "". I think it is always set, and the if statement is not needed
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.
I think you're right, but separate PR probably
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.
Can you pull this comment out of this PR?
cmd/kops/get_cluster.go
Outdated
@@ -215,6 +214,100 @@ func clusterOutputJson(clusters []*api.Cluster, out io.Writer) error { | |||
return nil | |||
} | |||
|
|||
// TODO refactor all output JSON methods to use this |
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.
Can we create two helpers:
func marshalMultipleToYaml(w io.Writer, obj runtime.Object...) error {
func marshalMultipleToJson(w io.Writer, obj runtime.Object...) error {
Should make this simpler
8133d21
to
13fc997
Compare
4185ca6
to
3ba6bfc
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.
Generally looks good, just a few small fixes
cmd/kops/create_cluster.go
Outdated
} | ||
|
||
var targetOptions = sets.NewString(cloudup.TargetCloudformation, cloudup.TargetDirect, cloudup.TargetTerraform) | ||
var allTargetOptions = strings.Join(targetOptions.List(), ",") |
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.
targetOptions
is unused? Can we put direct first, as it is the default?
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.
I will remove the set then ...
cmd/kops/create_cluster.go
Outdated
@@ -987,6 +1013,39 @@ func RunCreateCluster(f *util.Factory, out io.Writer, c *CreateClusterOptions) e | |||
return err | |||
} | |||
|
|||
if c.DryRun { | |||
var obj []runtime.Object | |||
// if we do not set the ts we get creationTimestamp: null |
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.
That's what kubectl does, so can you add a comment on the consequence of creationTimestamp null
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.
kubectl sets null, I think we should probably be doing the same
@@ -1008,6 +1065,7 @@ func RunCreateCluster(f *util.Factory, out io.Writer, c *CreateClusterOptions) e | |||
} | |||
} | |||
|
|||
// Can we acutally get to this if?? |
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.
I think you're right, but separate PR probably
cmd/kops/create_ig.go
Outdated
@@ -39,6 +40,11 @@ import ( | |||
type CreateInstanceGroupOptions struct { | |||
Role string | |||
Subnets []string | |||
// DryRun we output data |
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.
Comment is a little obscure
cmd/kops/get.go
Outdated
obj = append(obj, group) | ||
} | ||
if err := fullOutputJSON(out, obj...); err != nil { | ||
return fmt.Errorf("error writing cluster yaml to stdout: %v", err) |
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.
json
cmd/kops/get.go
Outdated
return err | ||
}*/ | ||
|
||
var obj []runtime.Object |
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.
Hoist out of the loop please
cmd/kops/get_cluster.go
Outdated
|
||
default: | ||
return fmt.Errorf("Unknown output format: %q", options.output) | ||
return fmt.Errorf("unknown output format: %q", options.output) |
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.
So the reason for these being upper case is because they look better when displayed, and this is a CLI command object. It's the same way the go rules conflict with all our comments in apimachinery types.
I guess we should format the error forcing the first character to upper-case? But in another PR. Please no more case-switches in this PR though.
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.
I will back them out, they bug me
func fullOutputJSON(out io.Writer, args ...runtime.Object) error { | ||
argsLen := len(args) | ||
|
||
if argsLen > 1 { |
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.
I think we should always write the array, even if it is empty. I don't think an empty string is valid JSON?
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.
JSON object is not an array. For instance when we create an ig, we just get a single object
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 can always write an array, but at least with a single JSON object create -f works, with a JSON array it bombs
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 discussed this; you are right - better to only write the array if there are multiple members. This way a single object is an object
cmd/kops/get_cluster.go
Outdated
return nil | ||
} | ||
|
||
func clusterOutputYAML(clusters []*api.Cluster, out io.Writer) error { | ||
for i, cluster := range clusters { | ||
// fullOutputJson outputs the marshalled YAML of a list of clusters and instance groups. It will handle |
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.
yaml
937d32f
to
66e6f95
Compare
Looks good, we just need to figure out if creationTimestamp should be set... I think not? |
66e6f95
to
d7af014
Compare
Added --dry-run for create_ig and create_cluster
d7af014
to
4c82a6d
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue. |
Allowing a user to create a YAML or JSON cluster or instance group without creating the object. Some of the new methods will be used to fix the problems we are having with JSON output as well. Reading an array of JSON objects is not yet supported by
kops create -f
, but a single JSON object is supported.This implements
kops create cluster --dry-run -oyaml
TODOs