linode: Add VPC cloudup task#18316
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/retest |
hakman
left a comment
hakman
left a comment
There was a problem hiding this comment.
@moshevayner nice progress, some comments, mostly small stuff.
additionally, please look into adding the OWNERS files to the new packages.
| func (c *Cloud) AccessToken() string { | ||
| return c.accessToken | ||
| } |
There was a problem hiding this comment.
This method is required in order to maintain the interface contract. When I tried to remove it, I got a compliation error:
cannot use &Cloud{…} (value of type *Cloud) as LinodeCloud value in return statement: *Cloud does not implement LinodeCloud (missing method AccessToken)
There was a problem hiding this comment.
did you remove both at the same time?
There was a problem hiding this comment.
I did, but I still had compilations error. Ended up removing from the struct as well and that fixed the errors. Should be good now.
| func (c *MockLinodeCloud) AccessToken() string { | ||
| return c.AccessToken_ | ||
| } |
| } | ||
|
|
||
| client := linodego.NewClient(nil) | ||
| client.SetUserAgent("kops") |
There was a problem hiding this comment.
adding the version is easy and useful for debugging later
| client.SetUserAgent("kops") | |
| client.SetUserAgent("kops/" + version.Version) |
| if found != nil { | ||
| return nil, fmt.Errorf("found multiple Linode (Akamai) VPCs named %q", name) | ||
| } | ||
| found = candidate |
There was a problem hiding this comment.
I intentionally kept scanning so we can detect duplicate matching VPCs rather than taking the first one. Breaking here would hide ambiguous matches.
If you think that's an overkill, I'll break here instead.
| } | ||
|
|
||
| var found *linodego.VPC | ||
| if v.ID != nil { |
There was a problem hiding this comment.
is there any case where you have the id?
There was a problem hiding this comment.
Actually, not one that I can think of.. Overkill, I'll drop this branch.
hakman
changed the title
moshevayner
force-pushed
the
linode-pr-vpc
branch
4 times, most recently
from
7c35da0 to
6ca2cdd
Compare
Adds the first Linode cloudup infrastructure task: VPC create/update support. This intentionally does not add Linode instances, volumes, load balancers, DNS, or full cluster bring-up support. Those should land in follow-up PRs. Signed-off-by: Moshe Vayner <moshe@vayner.me>
moshevayner
force-pushed
the
linode-pr-vpc
branch
from
6ca2cdd to
ee2d9ef
Compare
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
3 participants
Adds the first Linode cloudup infrastructure task: VPC create/update support.
This intentionally does not add Linode instances, volumes, load balancers, DNS, or full cluster bring-up support. Those should land in follow-up PRs.
Signed-off-by: Moshe Vayner moshe@vayner.me
/cc @hakman
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):Special notes for your reviewer: