Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(config): Allow canary config id to be passed in instead of alway… #611

Merged
merged 1 commit into from
Sep 7, 2019
Merged

feat(config): Allow canary config id to be passed in instead of alway… #611

merged 1 commit into from
Sep 7, 2019

Conversation

duftler
Copy link
Collaborator

@duftler duftler commented Sep 6, 2019

…s randomly generated.

dotdotdotpaul
dotdotdotpaul approved these changes Sep 6, 2019
View reviewed changes
Copy link
Collaborator

@dotdotdotpaul dotdotdotpaul left a comment

Choose a reason for hiding this comment

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

I like.

duftler
duftler commented Sep 6, 2019
View reviewed changes
kayenta-core/src/main/java/com/netflix/kayenta/canary/CanaryConfig.java Outdated
@@ -39,6 +39,8 @@

@NotNull @Getter @Setter private String name;

@NotNull @Getter @Setter private String id;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to add id to canary config, and it will be populated anytime the config is persisted.

It still does not need to be provided on a POST or a PUT, so no changes are required to any dependent services.

jtk54
jtk54 approved these changes Sep 6, 2019
View reviewed changes
Copy link

@jtk54 jtk54 left a comment

Choose a reason for hiding this comment

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

Few nits, LGTM. Thanks for the quick turnaround.

kayenta-web/src/main/java/com/netflix/kayenta/controllers/CanaryConfigController.java Outdated
canaryConfigId = UUID.randomUUID() + "";

// Ensure that the canary config id is stored within the canary config itself.
if (StringUtils.isEmpty(canaryConfig.getId())) {
Copy link

Choose a reason for hiding this comment

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

This if seems unnecessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, copy/paste error.

kayenta-web/src/main/java/com/netflix/kayenta/controllers/CanaryConfigController.java Outdated

// Ensure that the canary config id is stored within the canary config itself.
if (StringUtils.isEmpty(canaryConfig.getId())) {
canaryConfig = canaryConfig.toBuilder().id(canaryConfigId).build();
Copy link

Choose a reason for hiding this comment

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

Don't you have an @Setter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shouldn't, will remove it. Trying to stick to not modifying the instance once it's constructed.

plumpy
plumpy approved these changes Sep 6, 2019
View reviewed changes
kayenta-web/src/main/java/com/netflix/kayenta/controllers/CanaryConfigController.java Outdated

// Ensure that the canary config id is stored within the canary config itself.
canaryConfig = canaryConfig.toBuilder().id(canaryConfigId).build();
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Probably just personal preference, but I think this would read easier if you removed the else block, so ti's more like

if (isEmpty(canaryConfig.getId())) {
  canaryConfig = canaryConfig.toBuilder().id(UUID.randomUUID().toString()).build();
}

String canaryConfigId = canaryConfig.getId();

Feel free to ignore if you disagree.

👍 to not modifying the object more than we already do... it's certainly surprising to pass an object to a store() method and find out the method secretly also modified it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That does read easier. Changed it.

@duftler
Copy link
Collaborator Author

duftler commented Sep 6, 2019

Closes https://github.com/spinnaker/kayenta/issues/610.

csanden
csanden approved these changes Sep 6, 2019
View reviewed changes
Copy link
Contributor

@csanden csanden left a comment

Choose a reason for hiding this comment

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

LGTM

fieldju
fieldju reviewed Sep 6, 2019
View reviewed changes
kayenta-core/src/main/java/com/netflix/kayenta/canary/CanaryConfig.java Outdated
@@ -39,6 +39,8 @@

@NotNull @Getter @Setter private String name;

@NotNull @Getter private String id;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this field is not null, then you should populate this field on the GET requests to configs that already exist but do not yet have the id saved as part of the object.

kayenta/kayenta-web/src/main/java/com/netflix/kayenta/controllers/CanaryConfigController.java

Lines 61 to 79 in c2acf40

@RequestMapping(value = "/{canaryConfigId:.+}", method = RequestMethod.GET)
public CanaryConfig loadCanaryConfig(
@RequestParam(required = false) final String configurationAccountName,
@PathVariable String canaryConfigId) {
String resolvedConfigurationAccountName =
CredentialsHelper.resolveAccountByNameOrType(
configurationAccountName,
AccountCredentials.Type.CONFIGURATION_STORE,
accountCredentialsRepository);
StorageService configurationService =
storageServiceRepository
.getOne(resolvedConfigurationAccountName)
.orElseThrow(
() ->
new IllegalArgumentException(
"No configuration service was configured; unable to read canary config from bucket."));
return configurationService.loadObject(
resolvedConfigurationAccountName, ObjectType.CANARY_CONFIG, canaryConfigId);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this should be @nullable, so that way if JSR 308 validation is turned on people who don't supply an id, since it's technically optional won't get an API error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Have removed @NotNull.

@duftler duftler merged commit c29e8d9 into spinnaker:master Sep 7, 2019
@duftler duftler deleted the pass-in-config-id branch September 7, 2019 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fieldju fieldju fieldju left review comments

@dotdotdotpaul dotdotdotpaul dotdotdotpaul approved these changes

@plumpy plumpy plumpy approved these changes

@csanden csanden csanden approved these changes

@nisanharamati nisanharamati nisanharamati approved these changes

@jtk54 jtk54 jtk54 approved these changes

Assignees
No one assigned
Labels
target-release/1.17
Projects
None yet
Milestone
No milestone
8 participants
@duftler @dotdotdotpaul @plumpy @fieldju @csanden @nisanharamati @jtk54 @spinnakerbot