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

fix: Allow all resources to add external links #7923

Merged
merged 1 commit into from
Jan 18, 2022

Conversation

tyrken
Copy link
Contributor

@tyrken tyrken commented Dec 14, 2021

Improves the phase 1 implementation of #3487 to allow adding links via annotations to all resource types, including ingresses/pods/services that previously skipped looking for annotations.

  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

@tyrken
Copy link
Contributor Author

tyrken commented Dec 14, 2021

FWIW I've cherry-picked this change on top of the v2.1.7 we run locally, and it seems to work. Happy to open a second PR to copy the change to the release-2.2 branch or whatever is needed to get it in the next release...

Signed-off-by: Tristan Keen <tristan.keen@gmail.com>
@tyrken tyrken force-pushed the links-all-resources branch from e0a4399 to 540b2cf Compare January 14, 2022 11:49
@tyrken
Copy link
Contributor Author

tyrken commented Jan 14, 2022

I've rebased & added another test - any chance of review or at least letting CI run @alexmt ? (Sorry don't know who else to ask - no response on CNCF slack...)

@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #7923 (540b2cf) into master (2f83312) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7923      +/-   ##
==========================================
- Coverage   41.53%   41.52%   -0.01%     
==========================================
  Files         174      174              
  Lines       22705    22700       -5     
==========================================
- Hits         9430     9427       -3     
+ Misses      11921    11918       -3     
- Partials     1354     1355       +1     
Impacted Files Coverage Δ
controller/cache/info.go 65.90% <ø> (+0.13%) ⬆️
util/settings/settings.go 47.01% <0.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 2f83312...540b2cf. Read the comment docs.

@jannfis
Copy link
Member

jannfis commented Jan 15, 2022

Thanks for your PR and your patience, @tyrken - I'll take a closer look at this one ASAP.

Copy link
Collaborator

@alexmt alexmt 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 PR @tyrken ! Change totally makes sense. LGTM.

@jannfis do you mind if I merge it?

Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

LGTM as well

@alexmt alexmt merged commit 2fb3986 into argoproj:master Jan 18, 2022
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