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.

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(timezone): Makes timezone CLI-configurable #557

Merged
merged 1 commit into from
Jun 13, 2017

Conversation

lwander
Copy link
Member

@lwander lwander commented Jun 13, 2017

No description provided.

Copy link

@duftler duftler left a comment

Choose a reason for hiding this comment

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

LGTM after addressing comments.

DeploymentConfiguration test = deploymentConfigurations.get(i);
if (test.getName().equals(deploymentName)) {
deploymentConfigurations.set(i, deploymentConfiguration);
return;
Copy link

Choose a reason for hiding this comment

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

Might read easier to have one block to locate the index of the matching config, and then another to set the new config. As it stands now, it looks like you are setting a new config while you are still searching through the list.

String timezone = n.getTimezone();

if (Arrays.stream(TimeZone.getAvailableIDs()).noneMatch(t -> t.equals(timezone))) {
p.addProblem(Problem.Severity.WARNING, "Timezone " + timezone + " does not match any known canonical timezone ID");
Copy link

Choose a reason for hiding this comment

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

Maybe provide a link to the list of canonical timezone ids?

@lwander lwander merged commit 3a0174d into spinnaker:master Jun 13, 2017
@lwander lwander deleted the timezone branch June 13, 2017 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants