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.

Sign up for GitHub

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

Jump to bottom

Add providerID #274

Closed
wants to merge 1 commit into from
Closed

Add providerID #274

wants to merge 1 commit into from

Conversation

jichenjc
Copy link
Contributor

refer to aws implementation, add providerID in exists function

What this PR does / why we need it:

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

Special notes for your reviewer:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 25, 2019
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 25, 2019
@jichenjc
Copy link
Contributor Author

need upgrade cluster-api.......

@chrigl
Copy link

chrigl commented Mar 25, 2019

/hold

Just to not being merged accidentally. As @jichenjc mentioned... we need an update of cluster-api for this.

With having this, we might want to remove the annotation openstack-resourceId, because in fact this is the same.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 25, 2019
@jichenjc
Copy link
Contributor Author

@chrigl can you share how to upgrade cluster-api? seems change the master branch to latest commit doesn't help, should we wait for a release of cluster-api?

@chrigl Octobox
Copy link

chrigl commented Mar 25, 2019

@jichenjc We keep the master branch of cluster-api, since there is no release at the horizon. You could update it using dep ensure --update sigs.k8s.io/cluster-api... But the hard part is testing it. And please do it in a separate branch.

@chrigl chrigl mentioned this pull request Apr 3, 2019
Closed
@jichenjc jichenjc mentioned this pull request Apr 28, 2019
Closed
@jichenjc jichenjc force-pushed the add_provider_id branch from a576674 to 1099e15 Compare May 1, 2019 03:21
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 1, 2019
@jichenjc jichenjc force-pushed the add_provider_id branch from 1099e15 to 00cb060 Compare May 1, 2019 03:38
@jichenjc
Add providerID
abb2c96 
refer to aws implementation, add providerID in exists function
@jichenjc
Copy link
Contributor Author

@chrigl because we already updated the cluster-api to branch 0.1.0 , we can remove the /hold flag here? Thanks

@frobware
Copy link

I plan to update kubernetes/autoscaler#1866 so that mapping between nodes and machines relies solely on the ProviderID being set -- this PR would help that goal.

@frobware frobware mentioned this pull request May 28, 2019
Merged
@chrigl
Copy link

chrigl commented Jul 11, 2019

@jichenjc Could you fit that into the refactor of #383 (which is already in master)?

@texascloud
Copy link

What's the hold up on merging this?

@jichenjc
Copy link
Contributor Author

we should already have this provider ID in latest code already

@jichenjc jichenjc closed this Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrigl chrigl Awaiting requested review from chrigl

@roberthbailey roberthbailey Awaiting requested review from roberthbailey

Assignees
No one assigned
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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jichenjc @chrigl @k8s-ci-robot @frobware @texascloud