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

feat: support configurable tags and enableMultipleStandardLoadBalancers #4048

Merged

Conversation

nilo19
Copy link
Member

@nilo19 nilo19 commented Nov 18, 2020

Reason for Change:

Issue Fixed:

Credit Where Due:

Does this change contain code from or inspired by another project?

  • No
  • Yes

If "Yes," did you notify that project's maintainers and provide attribution?

  • No
  • Yes

Requirements:

Notes:

Fixes: #4035

@acs-bot acs-bot added the size/L label Nov 18, 2020
@nilo19 nilo19 force-pushed the feature/support-cloud-provider-config branch from 5e09f81 to 1264ac3 Compare November 18, 2020 12:16
@jackfrancis
Copy link
Member

Thanks @nilo19

I think we should merge this support w/ cluster-api Azure day-and-date with aks-engine

@CecileRobertMichon @devigned are you aware of these 2 upstream changes that this aks-engine PR supports?

kubernetes/kubernetes#96111
kubernetes/kubernetes#96450

@CecileRobertMichon
Copy link
Contributor

CAPZ supports BYO azure.json secret already so no code changes should be needed to try this today.

In the future, if this is a common configuration, we should consider exposing them in the spec to make UX a little smoother for users who don't want to have to provide the entire config, similar to kubernetes-sigs/cluster-api-provider-azure#815.

@nilo19 nilo19 force-pushed the feature/support-cloud-provider-config branch from 1264ac3 to 937a4d5 Compare November 23, 2020 01:41
@mboersma
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nilo19 nilo19 force-pushed the feature/support-cloud-provider-config branch from 937a4d5 to f254221 Compare November 24, 2020 02:07
@mboersma
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov
Copy link

codecov bot commented Nov 25, 2020

Codecov Report

Merging #4048 (eefcb9b) into master (9d63eaf) will increase coverage by 0.12%.
The diff coverage is 96.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4048      +/-   ##
==========================================
+ Coverage   73.25%   73.37%   +0.12%     
==========================================
  Files         135      135              
  Lines       20671    20859     +188     
==========================================
+ Hits        15142    15306     +164     
- Misses       4552     4576      +24     
  Partials      977      977              
Impacted Files Coverage Δ
pkg/api/vlabs/types.go 73.04% <ø> (ø)
pkg/engine/templates_generated.go 44.19% <ø> (+0.49%) ⬆️
pkg/api/vlabs/validate.go 80.73% <50.00%> (-1.12%) ⬇️
pkg/api/converterfromapi.go 95.69% <100.00%> (+0.01%) ⬆️
pkg/api/convertertoapi.go 94.05% <100.00%> (+0.02%) ⬆️
pkg/api/types.go 92.74% <100.00%> (+0.02%) ⬆️
cmd/upgrade.go 36.45% <0.00%> (-1.31%) ⬇️
pkg/engine/masterarmresources.go 67.16% <0.00%> (-1.02%) ⬇️
pkg/engine/params_k8s.go 80.13% <0.00%> (-0.56%) ⬇️
... and 10 more

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 9d63eaf...eefcb9b. Read the comment docs.

@nilo19
Copy link
Member Author

nilo19 commented Nov 28, 2020

Do we plan to merge this?

@jackfrancis jackfrancis force-pushed the feature/support-cloud-provider-config branch from f254221 to c2b2976 Compare December 2, 2020 17:55
@jackfrancis
Copy link
Member

@nilo19 I rebased on top of current master branch, and added some E2E test foo to ensure that we test this new functionality.

How can we test the tags functionality?

@nilo19
Copy link
Member Author

nilo19 commented Dec 3, 2020

@jackfrancis If the tags are set in azure.json, then it functions well.

@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nilo19 nilo19 force-pushed the feature/support-cloud-provider-config branch from 19f98e6 to 4b626e0 Compare January 5, 2021 01:53
@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jackfrancis
Copy link
Member

There's one more thing before we can merge this PR, we need pkg/api/converterfromapi.go updated as well.

@nilo19 nilo19 force-pushed the feature/support-cloud-provider-config branch from 4b626e0 to eefcb9b Compare January 28, 2021 03:10
@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

/lgtm

@acs-bot
Copy link

acs-bot commented Feb 9, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, nilo19

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@jackfrancis jackfrancis merged commit 9924649 into Azure:master Feb 9, 2021
@nilo19 nilo19 deleted the feature/support-cloud-provider-config branch February 10, 2021 02:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support cloud provider config enableMultipleStandardLoadBalancers and tags
5 participants