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

Support Ingress from Networking API version #1529

Merged
merged 13 commits into from
Oct 26, 2021
Merged

Support Ingress from Networking API version #1529

merged 13 commits into from
Oct 26, 2021

Conversation

leoluz
Copy link
Contributor

@leoluz leoluz commented Sep 21, 2021 •
edited
Loading

Overview

This is related to #1507

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

@leoluz leoluz mentioned this pull request Sep 21, 2021
Closed
@leoluz leoluz force-pushed the leoluz/dev/1507 branch 3 times, most recently from 26be242 to de0b43b Compare September 24, 2021 16:30
@codecov
Copy link

codecov bot commented Sep 24, 2021 •
edited
Loading

Codecov Report

Merging #1529 (e811c36) into master (a398ff9) will increase coverage by 0.17%.
The diff coverage is 90.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1529      +/-   ##
==========================================
+ Coverage   81.58%   81.76%   +0.17%     
==========================================
  Files         112      113       +1     
  Lines       15169    15563     +394     
==========================================
+ Hits        12376    12725     +349     
- Misses       2136     2176      +40     
- Partials      657      662       +5
Impacted Files Coverage Δ
rollout/trafficrouting/nginx/nginx.go 80.19% <75.94%> (-5.62%) ⬇️
utils/ingress/wrapper.go 89.25% <89.25%> (ø)
rollout/trafficrouting/alb/alb.go 84.00% <92.85%> (-0.87%) ⬇️
ingress/alb.go 96.25% <100.00%> (+0.47%) ⬆️
ingress/ingress.go 60.60% <100.00%> (+0.40%) ⬆️
.../apis/rollouts/validation/validation_references.go 84.58% <100.00%> (ø)
rollout/controller.go 74.66% <100.00%> (ø)
rollout/trafficrouting.go 68.61% <100.00%> (ø)
utils/ingress/ingress.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a398ff9...e811c36. Read the comment docs.

@leoluz
feat: add unit-tests to the wrapper.go
be512ab 
Signed-off-by: Leonardo Luz Almeida <[email protected]>
@alexmt alexmt requested review from jessesuen and alexmt October 4, 2021 17:48
leoluz added 2 commits October 4, 2021 14:37
@leoluz
Trigger Build
02061da 
Signed-off-by: Leonardo Luz Almeida <[email protected]>
@leoluz
Trigger Build
a4c0711 
Signed-off-by: Leonardo Luz Almeida <[email protected]>
jessesuen
jessesuen reviewed Oct 20, 2021
View reviewed changes
utils/defaults/defaults.go Outdated
Comment on lines 283 to 290

func GetIngressAPIVersion() string {
return ingressAPIVersion
}

func SetIngressAPIVersion(apiVersion string) {
ingressAPIVersion = apiVersion
}
Copy link
Member

Choose a reason for hiding this comment

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

I have a proposal to make this easier for users to improve the UX. Instead of making users explicitly specify this, could we instead have a function called:

func DetermineIngressAPIVersion()

DetermineIngressAPIVersion() would be called in main.go and the way it would work is that it would first attempt a list on networking.io, but if that errors with resource not found, it would fall back to extensions.

I think we want to stem off all of the support/questions that we get where we have to tell people to use the flag. We can still have the flag, in case people want to explicitly use a specific version for some reason, but I think the default behavior should be to automatically pick for the user.

Copy link
Contributor Author

@leoluz leoluz Oct 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Good idea. I'm thinking that just instead of doing a list attempt, implementing it by checking the server version with discovery.ServerVersionInterface. Everything above 1.19 will automatically use the NetworkIngress mode. How that sounds?

@avg07 avg07 mentioned this pull request Oct 22, 2021
Closed
@leoluz
address review comments
f18f2a8 
Signed-off-by: Leonardo Luz Almeida <[email protected]>
@leoluz
Copy link
Contributor Author

leoluz commented Oct 22, 2021

@jessesuen just pushed a new commit implementing the improvements. Pls take a look.

@harikrongali harikrongali added the ready-for-review Ready for final review label Oct 22, 2021
jessesuen
jessesuen reviewed Oct 23, 2021
View reviewed changes
rollout/controller_test.go Outdated Show resolved Hide resolved
rollout/controller_test.go Outdated Show resolved Hide resolved
@leoluz leoluz requested a review from jessesuen October 25, 2021 14:23
jessesuen
jessesuen approved these changes Oct 26, 2021
View reviewed changes
Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

LGTM. There's one conflict that happened after merging in another PR but otherwise loogs good to go.

@leoluz leoluz requested a review from jessesuen October 26, 2021 01:54
@leoluz
trigger build
e811c36 
Signed-off-by: Leonardo Luz Almeida <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Oct 26, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 31 Code Smells

No Coverage information No Coverage information
4.4% 4.4% Duplication

@jessesuen jessesuen merged commit ac155f1 into argoproj:master Oct 26, 2021
kiranmeduri added a commit to kiranmeduri/argo-rollouts that referenced this pull request Oct 27, 2021
@kiranmeduri
fix: Handle minor version with '+' when determining ingress mode (arg…
fc642ca 
…oproj#1529)

Signed-off-by: Kiran Meduri <[email protected]>
jessesuen pushed a commit that referenced this pull request Oct 28, 2021
@kiranmeduri
fix: Handle minor version with '+' when determining ingress mode (#1529…
fe2c68b 
…) (#1612)

Signed-off-by: Kiran Meduri <[email protected]>
agrawroh pushed a commit to agrawroh/argo-rollouts that referenced this pull request Oct 28, 2021
@kiranmeduri @agrawroh
fix: Handle minor version with '+' when determining ingress mode (arg…
6852b1f 
…oproj#1529) (argoproj#1612)

Signed-off-by: Kiran Meduri <[email protected]>
Signed-off-by: Rohit Agrawal <[email protected]>
agrawroh pushed a commit to agrawroh/argo-rollouts that referenced this pull request Oct 28, 2021
@kiranmeduri @agrawroh
fix: Handle minor version with '+' when determining ingress mode (arg…
1fdcb96 
…oproj#1529) (argoproj#1612)

Signed-off-by: Kiran Meduri <[email protected]>
Signed-off-by: Rohit Agrawal <[email protected]>
@kandeshvari kandeshvari mentioned this pull request Nov 12, 2021
Closed
@leoluz leoluz mentioned this pull request Jan 24, 2022
Closed
@leoluz leoluz added this to the v1.2 milestone Jan 27, 2022
danielm-codefresh pushed a commit to codefresh-io/argo-rollouts that referenced this pull request Jan 31, 2022
@leoluz @danielm-codefresh
feat: support Ingress from Networking API version (argoproj#1529)
a903de4 
Signed-off-by: Leonardo Luz Almeida <[email protected]>
noam-codefresh pushed a commit to codefresh-io/argo-rollouts that referenced this pull request Feb 9, 2022
@leoluz @ATGardner
feat: support Ingress from Networking API version (argoproj#1529)
56b99b5 
Signed-off-by: Leonardo Luz Almeida <[email protected]>
noam-codefresh pushed a commit to codefresh-io/argo-rollouts that referenced this pull request Feb 9, 2022
@kiranmeduri @ATGardner
fix: Handle minor version with '+' when determining ingress mode (arg…
807fb8e 
…oproj#1529) (argoproj#1612)

Signed-off-by: Kiran Meduri <[email protected]>
danielm-codefresh pushed a commit to codefresh-io/argo-rollouts that referenced this pull request Mar 2, 2022
@leoluz @danielm-codefresh
feat: support Ingress from Networking API version (argoproj#1529)
89ce984 
Signed-off-by: Leonardo Luz Almeida <[email protected]>
danielm-codefresh pushed a commit to codefresh-io/argo-rollouts that referenced this pull request Mar 9, 2022
@kiranmeduri @danielm-codefresh
fix: Handle minor version with '+' when determining ingress mode (arg…
cc7a3c4 
…oproj#1529) (argoproj#1612)

Signed-off-by: Kiran Meduri <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexmt alexmt Awaiting requested review from alexmt

@jessesuen jessesuen Awaiting requested review from jessesuen

Assignees
No one assigned
Labels
ready-for-review Ready for final review
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.
3 participants
@leoluz @jessesuen @harikrongali