Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

normalizeazureregion added in scale, upgrade, and deploy #2764

Merged
merged 13 commits into from
Apr 27, 2018

Conversation

elizabethhalper
Copy link
Contributor

@elizabethhalper elizabethhalper commented Apr 24, 2018

What this PR does / why we need it: It solves the issue outlined in: #2258

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #2258

Special notes for your reviewer:

If applicable:

  • documentation
  • unit tests
  • tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

Release note:

@ghost ghost assigned elizabethhalper Apr 24, 2018
@ghost ghost added the in progress label Apr 24, 2018
cmd/deploy.go Outdated
@@ -83,6 +84,11 @@ func newDeployCmd() *cobra.Command {
return deployCmd
}

// NormalizeAzureRegion returns a normalized Azure region with whilte spaces removed and converted to lower case
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in the comment whilte --> white

Copy link
Contributor

Choose a reason for hiding this comment

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

We could add this function to https://github.com/Azure/acs-engine/blob/master/pkg/helpers/helpers.go and re-use it here and in pkg/api

@codecov
Copy link

codecov bot commented Apr 25, 2018

Codecov Report

Merging #2764 into master will decrease coverage by 0.01%.
The diff coverage is 46.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2764      +/-   ##
=========================================
- Coverage   46.82%   46.8%   -0.02%     
=========================================
  Files          86      86              
  Lines       12773   12778       +5     
=========================================
  Hits         5981    5981              
- Misses       6248    6253       +5     
  Partials      544     544
Impacted Files Coverage Δ
cmd/scale.go 0% <0%> (ø) ⬆️
pkg/api/convertertoagentpoolonlyapi.go 28.11% <0%> (ø) ⬆️
cmd/upgrade.go 0% <0%> (ø) ⬆️
cmd/deploy.go 23.8% <0%> (-0.15%) ⬇️
pkg/helpers/helpers.go 14.28% <100%> (+5.19%) ⬆️
pkg/api/convertertoapi.go 56.51% <100%> (-0.11%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ffa4c0...3c31b1b. Read the comment docs.

@@ -9,3 +9,32 @@ func TestPointerToBool(t *testing.T) {
t.Fatalf("expected PointerToBool(true) to return *true, instead returned %#v", ret)
}
}

func TestIsRegionNormalized(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

for _, c := range cases {
result := NormalizeAzureRegion(c.input)
if c.expectedResult != result {
t.Fatalf("NormalizeAzureRegion returned unexpected result: expected %t but got %t", c.expectedResult, result)
Copy link
Contributor

Choose a reason for hiding this comment

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

%s instead of %t here since we're printing string types

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

lgtm

@CecileRobertMichon CecileRobertMichon merged commit b456302 into master Apr 27, 2018
@ghost ghost removed the in progress label Apr 27, 2018
@seanknox seanknox deleted the locationformatter branch August 10, 2018 23:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

acs-engine admits location format that causes failure much later
2 participants