-
Notifications
You must be signed in to change notification settings - Fork 0
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 adminx package for naming resources #37
Conversation
Pull Request Test Coverage Report for Build 10641385168Details
💛 - Coveralls |
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.
LGTM, with one small typo fix, and two opinions, to accept or reject at your discretion.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @stephen-soltesz)
internal/adminx/namer.go
line 13 at r1 (raw file):
} // GetProjectsPrefix returns a google could project resource name,
Typo: s/could/cloud/
internal/adminx/namer.go
line 21 at r1 (raw file):
// GetServiceAccountID returns a service account ID for this org, e.g. autonode-gcsrw-org. func (n *Namer) GetServiceAccountID(org string) string { return "autonode-gcsrw-" + org
I gather gcsrw stands for Google Cloud Storage Read Write, or similar? Could that be expanded a bit to be more explicit. Long service account names aren't really a problem, but its helpful, at least to me, to be able to eye an account name and know immediately from the name what it is for.... sort of like many of our alert names. :) This is just an opinion, nothing more.
internal/adminx/namer.go
line 38 at r1 (raw file):
// GetSecretID returns a secret ID for this org, e.g. autojoin-secret-org. func (n *Namer) GetSecretID(org string) string { return "autojoin-secret-" + org
To me, adding the word "secret" to a Secret seems redundant. I think this could be just "autojoin + org".
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.
Thank you for your review!
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @nkinkade)
internal/adminx/namer.go
line 13 at r1 (raw file):
Previously, nkinkade wrote…
Typo: s/could/cloud/
Good catch!
internal/adminx/namer.go
line 21 at r1 (raw file):
Previously, nkinkade wrote…
I gather gcsrw stands for Google Cloud Storage Read Write, or similar? Could that be expanded a bit to be more explicit. Long service account names aren't really a problem, but its helpful, at least to me, to be able to eye an account name and know immediately from the name what it is for.... sort of like many of our alert names. :) This is just an opinion, nothing more.
Sadly service account names are limited to 30 characters https://cloud.google.com/iam/docs/service-accounts-create#creating
This prefix is half of that - which is probably too much - I've shortened it to just "autonode-org".
I wholeheartedly agree with the opinion that explicit names are preferable to cryptic abbreviations - even putting "gcsrw" may be an anti-pattern here since it's easy enough to see what roles are assigned to the SA and those could change in time beyond gcsrw.
internal/adminx/namer.go
line 38 at r1 (raw file):
Previously, nkinkade wrote…
To me, adding the word "secret" to a Secret seems redundant. I think this could be just "autojoin + org".
I seem to like doing that - I've removed it and used a label more related to the content - "autojoin-serviceaccount-key-"+org
Secret names can be 255 characters. https://cloud.google.com/secret-manager/docs/reference/rpc/google.cloud.secretmanager.v1
This change adds a new package
adminx
with the first of several types needed to manage service accounts and secrets within GCP for the autojoin API.This change is