-
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
Adding openstack bakery (#3) #632
Conversation
Adding Openstack Bakery commands to halyard h/t @shazy792 @dylanbernhardt @MatthewKohner @msilpala
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 work guys, just a few comments
|
||
@Parameters(separators = "=") | ||
public class OpenstackAddBaseImageCommand extends AbstractAddBaseImageCommand{ | ||
protected String getProviderName() {return "openstack"; } |
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.
Formatting seems off here
names = "--remove-consul-config", | ||
description = "Removes currently configured consul config file." | ||
) | ||
private boolean removeConsulConfig; |
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.
These "-remove" style flags aren't needed. You can always clear with --consul-config=
.
result.setAuthUrl("http://default.com"); | ||
result.setFloatingIpPool("default_pool"); | ||
result.setInsecure(false); | ||
result.setNetworkId("xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"); |
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.
These defaults will be set any time someone initializes the bakery options. The way these read, I'm guessing they'd have to be cleared/overwritten each time for this to be useful. If that's the case, just leave the fields blank.
OpenstackAccount.OpenstackLbaasOptions lbaas = account.getLbaas(); | ||
String regions = account.getRegions(); | ||
// TODO: consul config does not yet exist for OpenStack |
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.
Where does it not exist? Could you link to an open bug in the component where it needs to be added in the TODO()
braces?
if (!StringUtils.endsWith(authUrl, "/v3")) { | ||
psBuilder.addProblem(Problem.Severity.WARNING, "You must use Keystone v3. The default auth url will be of the format IP:5000/v3."); | ||
} | ||
|
||
if (domainName == null || domainName.isEmpty()) { |
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.
All of these string checks can be replaced with StringUtils.isEmpty(..)
from org.apache.commons.lang3
.
return; | ||
} | ||
|
||
|
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.
Extra newline
boolean insecure = n.isInsecure(); | ||
List<OpenstackBaseImage> baseImages = n.getBaseImages(); | ||
|
||
if (StringUtils.isEmpty(authUrl) && |
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 is this acceptable?
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 kept this line so the bakery defaults can be null when an account is enabled and added, but bakery parameters aren't set yet.
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.
Ah, and there are no defaults that can be set which work out of the box?
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
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 makes sense, thanks
p.addProblem(Problem.Severity.ERROR, "No ssh username supplied for openstack base image."); | ||
} | ||
|
||
// TODO(shazy792) Add check to see if image actually exists on openstack instance(thum |
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.
Looks like the comment got cut off here
n.getAccounts().forEach(openstackAccount -> openstackAccountValidator.validate(p, openstackAccount)); | ||
|
||
new OpenstackBakeryDefaultsValidator(credentialsList, halyardVersion).validate(p, n.getBakeryDefaults()); | ||
|
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.
Extra newline
|
||
import com.beust.jcommander.Parameters; | ||
import com.netflix.spinnaker.halyard.cli.command.v1.config.providers.bakery.AbstractBaseImageCommand; | ||
|
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.
Most of our commands seem to have a little class-level comment describing what they do in very generic terms. Do we want that here? @edwinavalos
|
||
@Parameter( | ||
names = "--remove-user-data-file", |
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.
Yep, Lars explained a difference between these removes and others in halyard,only removes that edit a list should be implemented the halyard way is to set the variable to nothing
|
||
static final String SSH_USER_NAME_DESCRIPTION = "The ssh username for the baking configuration."; | ||
|
||
static final String REGION_DESCRIPTION = "The region for the baking configuration."; |
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.
@edwinavalos Does this argument support multiple regions separated by commas? If so it's likely worth calling that out here in the description. If not, thank you for educating 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.
No, for Base Image virtualization settings it only supports one region.
import com.netflix.spinnaker.halyard.config.model.v1.node.Account; | ||
import com.netflix.spinnaker.halyard.config.model.v1.node.LocalFile; | ||
import com.netflix.spinnaker.halyard.config.model.v1.node.Validator; | ||
import com.netflix.spinnaker.halyard.config.problem.v1.ConfigProblemSetBuilder; | ||
import lombok.Data; | ||
import lombok.EqualsAndHashCode; | ||
|
||
import java.util.ArrayList; |
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.
Unused import.
private String authUrl; | ||
private String username; | ||
private String password; | ||
private String projectName; | ||
private String domainName; | ||
private Boolean insecure = false; | ||
private String heatTemplateLocation; | ||
private String consulConfig; | ||
// private lbaasConfig; |
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.
Any reason not to just delete this line?
@@ -1,11 +1,15 @@ | |||
package com.netflix.spinnaker.halyard.config.model.v1.providers.openstack; | |||
|
|||
import com.netflix.spinnaker.halyard.config.model.v1.node.HasImageProvider; | |||
import com.netflix.spinnaker.halyard.config.model.v1.node.Provider; |
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.
Unused import.
@@ -77,7 +77,8 @@ private BaseImage getBaseImage(NodeFilter filter, String baseImageName) { | |||
} | |||
} | |||
|
|||
public BakeryDefaults getBakeryDefaults(String deploymentName, String providerName) { | |||
public BakeryDefaults | |||
getBakeryDefaults(String deploymentName, String providerName) { |
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.
Pretty sure this line-break was accidental.
.lbaasConfig(lbaasConfig) | ||
.userDataFile(userDataFile) | ||
.build(); | ||
if (openstackCredentials == null) { | ||
return; |
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 fairly certain that it's impossible for this value to be null at this point: Java constructors cannot return null, and the build() method only calls a constructor and returns its result.
Did this check come about in response to an error? Or does it simply conform to convention elsewhere within the spinnaker/halyard code base? I'm new here so this might be a silly question. @edwinavalos
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 agree that this code path can't be hit - what's the intent here? Unless .build()
can return null?
import com.netflix.spinnaker.halyard.config.model.v1.providers.openstack.OpenstackBaseImage; | ||
import com.netflix.spinnaker.halyard.config.problem.v1.ConfigProblemSetBuilder; | ||
import com.netflix.spinnaker.halyard.config.validate.v1.providers.google.GoogleBaseImageValidator; | ||
import com.netflix.spinnaker.halyard.core.problem.v1.Problem; |
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.
Three unused imports:
- GoogleNamedAccountCredentials
- OpenstackCredentials
- GoogleBaseImageValidator
@@ -124,7 +124,7 @@ protected void setProfile(Profile profile, DeploymentConfiguration deploymentCon | |||
if (openstackProvider.getPrimaryAccount() != null) { | |||
OpenstackAccount openstackAccount = (OpenstackAccount) accountService.getProviderAccount(deploymentConfiguration.getName(), "openstack", openstackProvider.getPrimaryAccount()); | |||
//Regions in openstack are a comma separated list. Use the first as primary. |
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 comment is out of date, isn't it? Remove or update. Regions are now a list of strings from command all the way to domain objects.
@edwinavalos I felt bad for putting in so many nit-sized remarks about unused imports, so I made a secondary PR you and use to just take all those changes en masse: https://github.com/edwinavalos/halyard/pull/6 |
* fix formatting * fix virtualization settings for openstack base image
@lwander this one is ready for a once over again |
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, just a few more comments
*/ | ||
@Parameters(separators = "=") | ||
public class OpenstackEditBakeryDefaultsCommand extends AbstractEditBakeryDefaultsCommand<OpenstackBakeryDefaults> { | ||
protected String getProviderName() { return "openstack"; } |
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'd prefer to have this on a new line to be consistent with other formatting
names = "--insecure", | ||
description = "Set the default security setting (true or false) to connect to the openstack account." | ||
) | ||
private boolean insecure = 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.
This should be nullable Boolean
to decide if it's been set or not (otherwise will always default to false
when a user makes an unrelated edit).
virtualizationSettings.setRegion(isSet(region) ? region : virtualizationSettings.getRegion()); | ||
virtualizationSettings.setInstanceType(isSet(instanceType) ? instanceType : virtualizationSettings.getInstanceType()); | ||
virtualizationSettings.setSshUserName(isSet(sshUserName) ? sshUserName : virtualizationSettings.getSshUserName()); | ||
// baseImage.setVirtualizationSettings(virtualizationSettings); |
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.
Please delete or uncomment
result.setFloatingIpPool(null); | ||
result.setInsecure(false); | ||
result.setNetworkId(null); | ||
result.setPassword(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.
Setting null
isn't needed, you can just return new OpenstackBakeryDefaults()
.lbaasConfig(lbaasConfig) | ||
.userDataFile(userDataFile) | ||
.build(); | ||
if (openstackCredentials == null) { | ||
return; |
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 agree that this code path can't be hit - what's the intent here? Unless .build()
can return null?
* remove extraneous if * fix intellij formatting * Make bakery insecure parameter nullable * remove commented out line * remove OpenstackBakeryDefaults null setting and make insecure required setting so vaildation works
I'm ready to merge this, any objections? |
no objections, merge away! |
Adding Openstack Bakery commands to halyard
h/t @shazy792 @dylanbernhardt @MatthewKohner @msilpala