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

Update linodego to v1 #118

Closed
wants to merge 4 commits into from

Conversation

okokes-akamai
Copy link
Contributor

@okokes-akamai okokes-akamai commented Feb 23, 2023

General:

  • Have you removed all sensitive information, including but not limited to access keys and passwords?
  • Have you checked to ensure there aren't other open or closed Pull Requests for the same bug/feature/question?

Pull Request Guidelines:

  1. Does your submission pass tests?
  2. Have you added tests?
  3. Are you addressing a single feature in this PR?
  4. Are your commits atomic, addressing one change per commit?
  5. Are you following the conventions of the language?
  6. Have you saved your large formatting changes for a different PR, so we can focus on your work?
  7. Have you explained your rationale for why this feature is needed?
  8. Have you linked your PR to an open issue

@okokes-akamai okokes-akamai changed the title Update linodego dependency Server side filtering of NodeBalancers (+ linodego bump) Feb 23, 2023
@okokes-akamai okokes-akamai marked this pull request as ready for review February 23, 2023 17:01
@okokes-akamai okokes-akamai changed the title Server side filtering of NodeBalancers (+ linodego bump) Update linodego to v1 Mar 9, 2023
@okokes-akamai
Copy link
Contributor Author

Once #126 gets merged, rebase this and add the following to getNodeBalancerByIPv4 instead of the hardcoded filter (this constructor is new to v1).

f := &linodego.Filter{}
f.AddField(linodego.Eq, "ipv4", ipv4)
filter, err := f.MarshalJSON()
if err != nil {
	return nil, err
}

@git-it127
Copy link

@okokes-akamai it seems this PR is not currently working. Can we convert this to a draft PR until it is in a working state.

@okokes-akamai
Copy link
Contributor Author

Bumped linodego in e2e deps as well, just haven't had a chance to test it as I have issues with my environment.

@luthermonson
Copy link
Contributor

@okokes-akamai if this is still good to go then let's get it merged. could you also bump go to 1.20 while youre in the go.mod?

@@ -6,10 +6,10 @@ require (
github.com/appscode/go v0.0.0-20200323182826-54e98e09185a
github.com/getsentry/sentry-go v0.4.0
github.com/golang/mock v1.6.0
github.com/linode/linodego v0.32.2
github.com/linode/linodego v1.14.1
Copy link
Contributor

Choose a reason for hiding this comment

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

think you could go all the way to v1.25.0 now?

@okokes-akamai
Copy link
Contributor Author

@okokes-akamai if this is still good to go then let's get it merged. could you also bump go to 1.20 while youre in the go.mod?

This is definitely not ready to be merged, because it has implications for our deployment model due to the linodego URL handling. So we cannot just merge it, because then we'd have to rework our internal systems accordingly (and would thus be blocked from developing CCM until that happens).

@okokes-akamai okokes-akamai marked this pull request as draft November 9, 2023 10:45
@rammanoj
Copy link
Contributor

This is being taken care at #143. @okokes-akamai I believe this should be good to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants