Skip to content

Conversation

@deads2k
Copy link
Contributor

@deads2k deads2k commented Mar 25, 2015

This properly segregate master cert generation from node cert generation. It eliminates unnecessary commands and starts properly passing command names and io.Writer

@liggitt

@deads2k deads2k force-pushed the deads-cleanup-admin-commands branch from c7c6173 to bcb6c46 Compare March 25, 2015 19:19
Copy link
Contributor

Choose a reason for hiding this comment

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

Pass in fullName, name like the other cli commands

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pass in fullName, name like the other cli commands

Done.

@deads2k deads2k force-pushed the deads-cleanup-admin-commands branch from bcb6c46 to 75f7dc1 Compare March 26, 2015 13:58
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you call them the same things the other ones do? fullName and name. And yes, the others are wrong if they're not like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like we'd want to pass name and baseName as opposed to name and fullName. Why would we want fullName instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

fullName is the name someone access you as in a larger context. Name is your name.

You are David. Your full name is David Eads.

----- Original Message -----

@@ -32,15 +33,15 @@ type CreateBootstrapPolicyFileOptions struct {
OpenShiftSharedResourcesNamespace string
}

-func NewCommandCreateBootstrapPolicyFile() *cobra.Command {
+func NewCommandCreateBootstrapPolicyFile(commandName string, baseName
string, out io.Writer) *cobra.Command {

Seems like we'd want to pass name and baseName as opposed to name and
fullName. Why would we want fullName instead?


Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1454/files#r27217531

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And if you were passing information to an API, you wouldn't want to pass FullName, you'd pass FirstName LastName (or GivenName SurName).

Should args be: "create-master-certs", "openshift admin create-master-certs" or "create-master-certs", "openshift admin". I vote for the latter and some commands are taking that, but calling the second arg "fullName" in spite of it being a "baseName".

Copy link
Contributor

Choose a reason for hiding this comment

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

On Mar 26, 2015, at 10:40 AM, David Eads [email protected] wrote:

In pkg/cmd/server/admin/create_bootstrappolicy_file.go:

@@ -32,15 +33,15 @@ type CreateBootstrapPolicyFileOptions struct {
OpenShiftSharedResourcesNamespace string
}

-func NewCommandCreateBootstrapPolicyFile() *cobra.Command {
+func NewCommandCreateBootstrapPolicyFile(commandName string, baseName string, out io.Writer) *cobra.Command {
And if you were passing information to an API, you wouldn't want to pass FullName, you'd pass FirstName LastName (or GivenName SurName).

Should args be: "create-master-certs", "openshift admin create-master-certs" or "create-master-certs", "openshift admin". I vote for the latter and some commands are taking that, but calling the second arg "fullName" in spite of it being a "baseName".

If you do that, every command has to do string manipulation. Stick to fullName name, but if you have a better naming scheme, do so.


Reply to this email directly or view it on GitHub.

@liggitt
Copy link
Contributor

liggitt commented Mar 27, 2015

Add --create-certs=false to test-cmd, use commandName in create_commands.go, then LGTM

@liggitt
Copy link
Contributor

liggitt commented Mar 27, 2015

I'll wait for this to go in on Monday before merging the https node PR

@deads2k deads2k force-pushed the deads-cleanup-admin-commands branch from 75f7dc1 to 69554e4 Compare March 30, 2015 12:53
@deads2k
Copy link
Contributor Author

deads2k commented Mar 30, 2015

@liggitt Comments seem to disappearing, but the create-node-config uses an https --listen because the server uses that by default and causes an all-in-one server without a specified config to expect to find node server certs. I think that splitting those parameters is outside the scope of what I'm trying to do here, so the https --listen will create the certs as expected, but still serve insecurely

@deads2k deads2k force-pushed the deads-cleanup-admin-commands branch from 69554e4 to 032d275 Compare March 30, 2015 13:01
@deads2k deads2k force-pushed the deads-cleanup-admin-commands branch from 032d275 to 28e7369 Compare March 30, 2015 13:07
@deads2k
Copy link
Contributor Author

deads2k commented Mar 30, 2015

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1341/) (Image: devenv-fedora_1163)

@liggitt
Copy link
Contributor

liggitt commented Mar 30, 2015

asset fix merged, re[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin up to 28e7369

openshift-bot pushed a commit that referenced this pull request Mar 30, 2015
@openshift-bot openshift-bot merged commit cd0f0a8 into openshift:master Mar 30, 2015
@deads2k deads2k deleted the deads-cleanup-admin-commands branch March 31, 2015 18:52
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.

4 participants