-
Notifications
You must be signed in to change notification settings - Fork 704
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
feat(bootstrapping): Adds --bootstrap-only flag to accounts, which re… #729
Conversation
480842a
to
73b8008
Compare
names = "--bootstrap-only", | ||
description = AccountCommandProperties.BOOTSTRAP_ONLY | ||
) | ||
Boolean bootstrapOnly = false; |
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 set a default here, otherwise any edit that doesn't include this flag will turn off the --bootstrap-only mode.
.stream() | ||
.filter(a -> ((Account) a).getBootstrapOnly()) | ||
.collect(Collectors.toList()); | ||
if (bootstrapAccounts.size() >= 2) { |
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.
You're only validating that there is at most 1 bootstrap-only account per provider. You want to do this check in the Providers
class and validate across all accounts
.findFirst(); | ||
// Silly Java won't let me do this in the above chained call way. | ||
if (account.isPresent()) { | ||
provider.getAccounts().remove(account.get()); |
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.
You can with .map
on the optional
child = (Provider) children.getNext(); | ||
if (isBootstrappingProvider) { | ||
List bootstrapAccounts = (List) provider.getAccounts() | ||
.stream() |
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 rest of the project aligns these call chains two indent levels rather than against the first method call.
@@ -19,4 +19,5 @@ | |||
|
|||
public class AccountCommandProperties { | |||
public static final String REQUIRED_GROUP_MEMBERSHIP_DESCRIPTION = "A user must be a member of at least one specified group in order to make changes to this account's cloud resources."; | |||
public static final String BOOTSTRAP_ONLY = "A bootstrap-only account is the account in which Spinnaker itself is deployed. When true, this account will not be included the accounts managed by Spinnaker. Similarly, the bootstrap instance of Spinnaker will only contain this account. Only one account can be marked as bootstrap-only."; |
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 feels tedious that a user has to manually flag this account after they've (in a separate place in the config) already marked the account as being used for "bootstrapping". It might make more sense to have this command be a flag in teh DeploymentEnvironment
, so they can say something like hal deploy edit --account-name 'my-boostrapping-account' --bootstrap-only
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.
Also it gets confusing if they've configured their k8s account with a docker registry that would have to be flagged too - which halyard should automatically address.
73b8008
to
92a415e
Compare
I moved the Furthermore, this logic tries to identify and remove docker registries that do not have any associated container-based accounts. So if you add docker registry A, don't associate it with any thing, and enable |
cc @willgorman |
The DC/OS changes look fine to me. |
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.
Nice
@@ -0,0 +1,32 @@ | |||
/* | |||
* Copyright 2017 Netflix, Inc. |
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.
Career move?
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.
More like "CYA"
@@ -1,5 +1,5 @@ | |||
/* | |||
* Copyright 2016 Google, Inc. | |||
* Copyright 2017 Netflix, Inc. |
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 too
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.
moves the flagged account from the deployed Spinnaker instance
92a415e
to
36e359b
Compare
…moves the flagged account from the deployed Spinnaker instance.
cc: @edwinavalos This isn't exactly what your group was looking for, but it's a jumping off point to pare down the unnecessary docker registries.