fix: add a ClusterName property and use it for resources names by sylr · Pull Request #202 · Azure/aks-engine

sylr · 2018-12-26T09:47:44Z

What this PR does / why we need it:

Rebase a branch we used at my company to fix the name of the resources.

It adds a property clusterName which is used to prefix the name of all Azure resources, e.g. : clusterName = k8s-prod -> k8s-prod-master-0, k8s-prod-master-1 ... etc

If no clusterName is provided the prefix of the resources falls back to (k8s|aks)-<clusterId>.

Which issue this PR fixes : fixes #56

Special notes for your reviewer:

Release note:

Add a clusterName property which is used to prefix all Azure resources.

codecov · 2018-12-26T13:32:55Z

Codecov Report

Merging #202 into master will decrease coverage by 0.02%.
The diff coverage is 78.94%.

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
- Coverage   53.21%   53.19%   -0.03%     
==========================================
  Files          95       95              
  Lines       14230    14239       +9     
==========================================
+ Hits         7573     7574       +1     
- Misses       5991     5997       +6     
- Partials      666      668       +2

codecov · 2018-12-26T13:32:55Z

Codecov Report

Merging #202 into master will decrease coverage by 0.02%.
The diff coverage is 78.94%.

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
- Coverage   53.21%   53.19%   -0.03%     
==========================================
  Files          95       95              
  Lines       14230    14239       +9     
==========================================
+ Hits         7573     7574       +1     
- Misses       5991     5997       +6     
- Partials      666      668       +2

codecov · 2018-12-26T13:32:55Z

Codecov Report

Merging #202 into master will increase coverage by <.01%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
+ Coverage   53.25%   53.25%   +<.01%     
==========================================
  Files          95       95              
  Lines       14246    14259      +13     
==========================================
+ Hits         7586     7594       +8     
- Misses       5995     5998       +3     
- Partials      665      667       +2

tariq1890 · 2018-12-26T18:25:10Z

@sylr Looks like the PR has some changes to fix goimports errors. Can they be excluded from this PR?

sylr · 2018-12-26T18:54:09Z

Yes. I rebased this branch onto #204 to be able to run tests.

…

On 26 Dec 2018, at 19:25, Tariq Ibrahim ***@***.***> wrote: @sylr Looks like the PR has some changes to fix goimports errors. Can they be excluded from this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

sylr · 2018-12-26T20:12:02Z

@tariq1890 can you approve the e2e checks on circleci, I'll remove the goimports patch tomorrow.

pkg/helpers/helpers.go

tariq1890 · 2018-12-27T04:50:54Z

.circleci/config.yml

          no_output_timeout: "30m"
      - store_artifacts:
          path: /go/src/github.com/Azure/aks-engine/_logs
+      - store_artifacts:


Why do we need this? We had removed this as a part of this PR - Azure/acs-engine#4214

I added this for debug purposes.

Why did you remove it ?

@jackfrancis may be the right person to ask for this. I believe this was found as unnecessary since we would run our e2e's locally if we were to inspect the outputs.

let's revert adding this back - circle ci is not used anymore

pkg/api/types.go

tariq1890 · 2018-12-27T05:00:32Z

pkg/api/types.go

 }

+// GetPrefix returns resource prefix based on Properties
+func (p *Properties) GetPrefix() (ret string) {


GetPrefix is too vague, let's have something specific. GetClusterPrefix would work IMO. Also open to other names :)

GetClusterName ?

tariq1890 · 2018-12-27T05:01:22Z

@sylr Done!

sylr · 2018-12-27T18:04:33Z

/assign @tariq1890

tariq1890 · 2018-12-27T19:55:23Z

test/e2e/azure/cli.go

 		"--resource-group", a.ResourceGroup.Name,
 		"--template-file", e.Config.GeneratedTemplatePath,
-		"--parameters", e.Config.GeneratedParametersPath)
+		"--parameters", e.Config.GeneratedParametersPath,


Assuming these were added for debugging and not meant to be included in the PR.

Actually I think it would be good to merge this as it gives complete error messages if anything goes wrong instead of a correlation id you can't use.

acs-bot · 2018-12-28T13:41:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sylr
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: tariq1890

If they are not already assigned, you can assign the PR to them by writing /assign @tariq1890 in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jackfrancis · 2019-01-02T18:03:38Z

@sylr Can you kindly add a detailed description of the changes in the PR?

sylr · 2019-01-02T18:26:50Z

@jackfrancis done

jackfrancis · 2019-01-04T20:54:47Z

Thanks for this @sylr

The change as-is is breaking. E.g.:

Before:

"masterVMNamePrefix": "k8s-master-63250626-",

After:

"masterVMNamePrefix": "k8s-63250626-master-",

We should make as a requirement of this change that it be non-breaking when the clusterName property is not used.

sylr · 2019-01-07T11:20:25Z

@jackfrancis I disagree on this.

We shouldn't have resources with different name patterns.

jackfrancis · 2019-01-08T00:31:28Z

@sylr I'm not sure what you mean. The important thing, to preserve backwards-compatibility, is that any new change like this not effect previous usage patterns. In this specific case, we need to make sure that the vm name generation implementation produces the same output as it did before when this new feature is not enabled.

Signed-off-by: Sylvain Rabot <s.rabot@lectra.com>

stale · 2019-02-07T12:38:53Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

acs-bot added the size/M label Dec 26, 2018

sylr force-pushed the resources-renaming branch from 46bf3d4 to 698462d Compare December 26, 2018 11:51

acs-bot added size/L and removed size/M labels Dec 26, 2018

sylr force-pushed the resources-renaming branch 6 times, most recently from 07916b5 to e330a58 Compare December 26, 2018 13:22

sylr force-pushed the resources-renaming branch 4 times, most recently from 51519f5 to 46bf3d4 Compare December 26, 2018 15:50

acs-bot added size/M and removed size/L labels Dec 26, 2018

sylr force-pushed the resources-renaming branch from 46bf3d4 to dc15b94 Compare December 26, 2018 15:51

acs-bot added size/L and removed size/M labels Dec 26, 2018

tariq1890 reviewed Dec 27, 2018

View reviewed changes

pkg/helpers/helpers.go Outdated Show resolved Hide resolved

tariq1890 reviewed Dec 27, 2018

View reviewed changes

pkg/api/types.go Outdated Show resolved Hide resolved

tariq1890 reviewed Dec 27, 2018

View reviewed changes

sylr force-pushed the resources-renaming branch 2 times, most recently from 2e8cfbb to aa66793 Compare December 27, 2018 09:31

sylr changed the title ~~Add a clusterName property and use it for resources name~~ Add a ClusterName property and use it for resources names Dec 27, 2018

acs-bot assigned tariq1890 Dec 27, 2018

tariq1890 reviewed Dec 27, 2018

View reviewed changes

mboersma changed the title ~~Add a ClusterName property and use it for resources names~~ fix: add a ClusterName property and use it for resources names Dec 31, 2018

Sylvain Rabot added 2 commits January 8, 2019 11:37

Add a ClusterName property and use it for resources' names

20a5e36

Signed-off-by: Sylvain Rabot <s.rabot@lectra.com>

test: turn on debug output for create deployment

7235c32

Signed-off-by: Sylvain Rabot <s.rabot@lectra.com>

sylr force-pushed the resources-renaming branch from 639b118 to 7235c32 Compare January 8, 2019 10:37

stale bot added the stale label Feb 7, 2019

stale bot closed this Feb 14, 2019

This was referenced Feb 20, 2019

Resources name template #544

Closed

Resources name template #545

Closed

feat: use go template for cluster resource names #552

Closed

Conversation

sylr commented Dec 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 26, 2018

Codecov Report

Uh oh!

codecov bot commented Dec 26, 2018

Codecov Report

Uh oh!

codecov bot commented Dec 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tariq1890 commented Dec 26, 2018

Uh oh!

sylr commented Dec 26, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sylr commented Dec 26, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tariq1890 commented Dec 27, 2018

Uh oh!

sylr commented Dec 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acs-bot commented Dec 28, 2018

Uh oh!

jackfrancis commented Jan 2, 2019

Uh oh!

sylr commented Jan 2, 2019

Uh oh!

jackfrancis commented Jan 4, 2019

Uh oh!

sylr commented Jan 7, 2019

Uh oh!

jackfrancis commented Jan 8, 2019

Uh oh!

stale bot commented Feb 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sylr commented Dec 26, 2018 •

edited

Loading

codecov bot commented Dec 26, 2018 •

edited

Loading

sylr commented Dec 26, 2018 via email •

edited

Loading