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

Alicloud: Refactor LoadBalancerWhiteList to LoadBalancerACL #8304

Merged
merged 4 commits into from
Jun 22, 2020
Merged

Alicloud: Refactor LoadBalancerWhiteList to LoadBalancerACL #8304

merged 4 commits into from
Jun 22, 2020

Conversation

bittopaz
Copy link
Contributor

@bittopaz bittopaz commented Jan 10, 2020

This is part of #4127.

Changes:

  1. LoadBalancerWhiteList was created before Alicloud came up with LoadBalancerAccessControlList, and also the APIs of AccessControlList is more elegant
  2. This is the first step to migrate from aliyungo to official Alicloud SDK, since aliyungo is not actively maintained

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 10, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @bittopaz. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 10, 2020
@bittopaz bittopaz mentioned this pull request Jan 10, 2020
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 10, 2020
@rifelpet
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 10, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 11, 2020
@bittopaz
Copy link
Contributor Author

bittopaz commented May 11, 2020

This is still relevant. It would be super nice if someone could help review.
Thanks in advance.

@hakman
Copy link
Member

hakman commented Jun 17, 2020

@bittopaz Sorry for the delay here. Would it be ok for you to rebase and I could take a look at the changes after that to review them?

@hakman
Copy link
Member

hakman commented Jun 17, 2020

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 17, 2020
@bittopaz
Copy link
Contributor Author

@hakman Thanks you for looking at this.
I just rebased the changes to master branch.

@hakman
Copy link
Member

hakman commented Jun 18, 2020

Thanks @bittopaz. Will try to review this during the weekend.

@hakman
Copy link
Member

hakman commented Jun 18, 2020

Does this also migrate users from LoadBalancerWhiteLists to LoadBalancerAccessControlList?
I guess I mean to ask if this migration doesn't break anything from the user's point of view.

@bittopaz
Copy link
Contributor Author

@hakman this won’t break anything. It’s just a rename, existing whitelist would be found in the new named resource, so nothing would change.

And meanwhile, I guess I’m probably the only kops user with Alicloud. 😄

@hakman
Copy link
Member

hakman commented Jun 19, 2020

/test pull-kops-verify-gomod

@hakman
Copy link
Member

hakman commented Jun 19, 2020

As I cannot test this, but changes seem reasonable...
/lgtm
/assign @rifelpet

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2020
@rifelpet
Copy link
Member

I can't test it either but it looks good to me too. one last question, can we delete upup/pkg/fi/cloudup/alitasks/loadbalancerwhitelist.go and loadbalancerwhitelist_fitask.go now? Should that be in this PR or a separate PR?

@bittopaz
Copy link
Contributor Author

@rifelpet This is already deleted/renamed in a95d592.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2020
@rifelpet
Copy link
Member

Ah you're right, my apologies for missing that. Sorry, it looks like you'll need one last rebase.

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 22, 2020
@bittopaz
Copy link
Contributor Author

Rebased to resolve conflicts.

@rifelpet
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bittopaz, rifelpet

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2020
@k8s-ci-robot k8s-ci-robot merged commit abe4c12 into kubernetes:master Jun 22, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jun 22, 2020
@bittopaz bittopaz deleted the fix-lbacl branch June 22, 2020 03:18
@bittopaz bittopaz restored the fix-lbacl branch June 22, 2020 03:18
@bittopaz bittopaz deleted the fix-lbacl branch June 22, 2020 03:18
@bittopaz bittopaz restored the fix-lbacl branch June 22, 2020 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants