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

fix: create role assignment for master VMs #2583

Merged
merged 7 commits into from Jan 24, 2020

Conversation

dennis-benzinger-hybris
Copy link
Contributor

@dennis-benzinger-hybris dennis-benzinger-hybris commented Jan 15, 2020

Fixes #2373
Fixes #2367

@acs-bot acs-bot added the size/L label Jan 15, 2020
@dennis-benzinger-hybris dennis-benzinger-hybris changed the title Fix 2373 fix: 2373 Jan 15, 2020
@mboersma
Copy link
Member

mboersma commented Jan 15, 2020

Thanks! I changed the PR title to match the first commit, because that's more human-readable and will appear in the release notes.

@mboersma mboersma changed the title fix: 2373 fix: create role assignment for master VMs Jan 15, 2020
@mboersma
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jackfrancis
Copy link
Member

@dennis-benzinger-hybris I added some quick unit test coverage to the additions in types.go. If you enable contributions from maintainers on this PR I can push that up.

Or, you could add those UT yourself, thanks!

@dennis-benzinger-hybris
Copy link
Contributor Author

@dennis-benzinger-hybris I added some quick unit test coverage to the additions in types.go. If you enable contributions from maintainers on this PR I can push that up.

Oh, sorry :-). Didn't want to exclude you. I've activated the checkbox now.

@devigned
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dennis-benzinger-hybris
Copy link
Contributor Author

/azp run pr-e2e

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2583 in repo Azure/aks-engine

@devigned
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@acs-bot acs-bot added size/XL and removed size/L labels Jan 21, 2020
@jackfrancis
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 Jan 21, 2020

Codecov Report

Merging #2583 into master will increase coverage by 0.04%.
The diff coverage is 91.22%.

@@            Coverage Diff             @@
##           master    #2583      +/-   ##
==========================================
+ Coverage   71.79%   71.83%   +0.04%     
==========================================
  Files         131      131              
  Lines       24787    24844      +57     
==========================================
+ Hits        17795    17847      +52     
- Misses       5966     5970       +4     
- Partials     1026     1027       +1

@dennis-benzinger-hybris
Copy link
Contributor Author

@jackfrancis, I've added a test for createKubernetesMasterRoleAssignmentForAgentPools which should fix the code coverage. Please start the /azp run pr-e2e once more.

@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jackfrancis
Copy link
Member

/azp run pr-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dennis-benzinger-hybris
Copy link
Contributor Author

The latest job seems to have failed because of

Status=429 Code=\"OperationNotAllowed\" Message=\"The server rejected the request because too many requests have been received for this subscription.\"

for some Kubernetes versions.
(from https://dev.azure.com/aks-engine/AKS-Engine/_build/results?buildId=3747&view=logs&j=a48fcffa-d400-5086-fd55-6a393fb8fdb7&t=9d93e863-c26a-5b58-475a-eafe7efc9f3e)

@jackfrancis, could you please investigate this or restart the job?

I don't have access to the Azure subscriptions used in the tests.

@jackfrancis
Copy link
Member

@dennis-benzinger-hybris those results are not related to your change.

I've rebased this PR on top of current master to improve E2E test fidelity, and am running a broader set of cluster configuration tests against this branch, including back-compat for upgrade and scale. Hang tight, we're almost there! :)

And thank you again.

@dennis-benzinger-hybris
Copy link
Contributor Author

@jackfrancis, have you started the pipeline after your push?

@devigned
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

Thanks again @dennis-benzinger-hybris !

@acs-bot acs-bot added the lgtm label Jan 24, 2020
@jackfrancis jackfrancis merged commit a6e765e into Azure:master Jan 24, 2020
@acs-bot
Copy link

acs-bot commented Jan 24, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dennis-benzinger-hybris, jackfrancis

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

@dennis-benzinger-hybris
Copy link
Contributor Author

@jackfrancis, thanks for merging 👍 . Do you have any idea when there will be a release with this fix?

@jackfrancis
Copy link
Member

We will cut a v0.46.1 by tomorrow EOD -8 GMT at the latest.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants