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

Use non APIPA when assigning IP Address #4032

Merged
merged 9 commits into from
Jun 1, 2021

Conversation

jbvmio
Copy link
Contributor

@jbvmio jbvmio commented Mar 30, 2021

What this PR does:
Returns the first non Automatic Private IP if possible. Will return APIP as last resort.

Which issue(s) this PR fixes:
Fixes or addresses #4014

Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, if you get a chance can you look into adding a DCO signature to your commits. Also I'd like to make a suggestion.

suggestion: Instead of using two label breaking loops, consider pushing logic in the filterIPs for loop into a separate function. This will allow for two things:

  1. The code can be rewritten to not use label breaks
  2. It will be easy to add unit tests to the new function that replaces the filterIPs loop.

@jbvmio WDYT?

@pull-request-size pull-request-size bot added size/M and removed size/S labels Apr 16, 2021
@jbvmio
Copy link
Contributor Author

jbvmio commented Apr 16, 2021

Sounds good to me @jtlisi, appreciate the feedback.
The requested changes have been made, let me know how it looks when you get a chance.

Signed-off-by: Jimmy Bonds <[email protected]>
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

@jbvmio This looks good to merge! Can you add a change log entry?

Signed-off-by: Jimmy Bonds <[email protected]>
@jbvmio
Copy link
Contributor Author

jbvmio commented May 8, 2021

@jtlisi Change log entry added, thanks!

Signed-off-by: Jimmy Bonds <[email protected]>
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Jimmy Bonds <[email protected]>
@jbvmio jbvmio requested a review from pstibrany May 20, 2021 14:33
Signed-off-by: Jimmy Bonds <[email protected]>
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you. Let's please remove extra changelog entries that are not related to this PR before merging.

Signed-off-by: Jimmy Bonds <[email protected]>
@pstibrany
Copy link
Contributor

Thank you for addressing my feedback.

@pstibrany pstibrany enabled auto-merge (squash) June 1, 2021 15:09
@pstibrany pstibrany merged commit ddcaac2 into cortexproject:master Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants