-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
installconfig: Replace 'KeyPairName' with 'SSHKey', and fix golint, uuid #188
Conversation
yifan-gu
commented
Aug 27, 2018
- Replace 'KeyPairName' with 'SSHKey'.
- Fix golint.
- Use better UUID() implementation from upstream.
Region: region, | ||
KeyPairName: keyPairName, | ||
Region: region, | ||
SSHKey: sshKey, |
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 drop SSHKey, #127
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.
the sshkey has moved to admin section.
@wking Yeah, I was feeling like I missed something, that's it! |
pkg/asset/installconfig/clusterid.go
Outdated
@@ -19,7 +19,7 @@ func (a *clusterID) Dependencies() []asset.Asset { | |||
func (a *clusterID) Generate(map[asset.Asset]*asset.State) (*asset.State, error) { | |||
return &asset.State{ | |||
Contents: []asset.Content{ | |||
{Data: []byte(uuid.NewUUID().String())}, | |||
{Data: []byte(uuid.NewUUID())}, |
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.
Previous discussion starting here. I'd still prefer to stick to pborman/uuid and just use the random (version 4) UUID via []byte(uuid.New())
.
pkg/asset/installconfig/doc.go
Outdated
@@ -0,0 +1,2 @@ | |||
// Package installconfig generates the install config assets based on its dependencies |
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.
nit: you're missing a trailing period here.
And maybe add a note about these being the main settings we expect users to edit?
pkg/asset/stock/doc.go
Outdated
@@ -0,0 +1,2 @@ | |||
// Package stock defines a stock type that holds the instanciated object of the assets. |
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.
nit: My aspell dictionary and wiktionary have "instantiated" but not "instanciated".
pkg/types/doc.go
Outdated
@@ -0,0 +1,2 @@ | |||
// Package types defines the InstallConfig that contains user-tuneable options. |
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.
nit: Are other types going to go in here too? If it's just InstallConfig
, maybe we should move that type definition into pkg/asset/installconfig
.
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'm inclined to keep it here for now, as the pkg/asset
is more for generating assets internally for the installer, but the InstallConfig
is kinda exportable, and might be referenced by other projects like cluster-operator, etc. So I want to make it stay in a top level package for easier accessibility.
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.
...but the
InstallConfig
is kinda exportable...
So this package is intended just for that struct? Or do we expect other structs to end up here as well? If it's just this one struct, how about pkg/installconfig
for the struct (leaving the asset logic deeper in pkg/asset/installconfig
)?
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.
Currently I am only seeing one, but I like to have pkg/types
, still more intuitive to me, even if I don't know what installconfig is, I will probably look into this package and find out.
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.
Currently I am only seeing one, but I like to have
pkg/types
...
So maybe the scope is:
// Package types defines structures for user-supplied installer configuration.
without it being InstallerConfig
specific?
@@ -270,6 +268,7 @@ type InstallConfig struct { | |||
type Admin struct { | |||
Email string `json:"email"` | |||
Password string `json:"password"` | |||
SSHKey string `json:"sshKey"` |
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.
Indent is wonky here. Maybe tabs vs. spaces?
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.
updated to spaces, seems like that's what other fields do.
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.
fixed other places too in the file.
594fe6f
to
6357eda
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.
A few more minor nits. I'm fine with these getting fixed here, and also with 6357eda landing as it stands and punting the nits to follow-up work.
pkg/asset/installconfig/doc.go
Outdated
@@ -0,0 +1,4 @@ | |||
// Package installconfig generates the install config assets based on its dependencies. | |||
// The definition of install config is at pkg/types/installconfig.go, it contains all |
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.
nit: pkg/types/installconfig.go
is file-level detail, but folks reading this in godoc.org will only care about package-level detail. Maybe something like:
// Package installconfig generates the install config assets based on its dependencies.
// The type itself is defined in ../pkg/types.
Or end with the full import path "...defined in github.com/openshift/installer/pkg/types.".
pkg/asset/stock/doc.go
Outdated
@@ -0,0 +1,2 @@ | |||
// Package stock defines a stock type that holds the instantiated object of the assets. |
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.
nit: Maybe just end with "...that holds the instantiated assets."?
pkg/types/installconfig.go
Outdated
@@ -45,6 +45,8 @@ type Admin struct { | |||
Email string `json:"email"` | |||
// Password is the password of the admin user. | |||
Password string `json:"password"` | |||
// SSHKey to use for the access to compute instances. | |||
SSHKey string `json:"sshKey"` |
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.
nit: Should this optional type be omitempty
?
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.
why this is optional?
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.
why this is optional?
Maybe the user doesn't care about SSH access? But mostly because it's omitempty
here, and I'd rather stay consistent ;).
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.
ok, updated
As now we will just accept the SSHKey from the prompt instead of a KeyPairName.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wking, yifan-gu 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 |
Signed-off-by: Alex Yang <yangyang1@zte.com.cn>