8000 feat: optionally propagate node labels to application pod view (#15274) by msoderberg · Pull Request #23260 · argoproj/argo-cd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: optionally propagate node labels to application pod view (#15274) #23260

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
Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

feat: optionally propagate node labels to application pod view (#15274) #23260

wants to merge 8 commits into from

Conversation

msoderberg
Copy link
Contributor
@msoderberg msoderberg commented Jun 4, 2025 •
edited
Loading

This PR adds support for optionally propagating node labels to the application pod view.

Closes #15274

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • 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).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@msoderberg msoderberg requested review from a team as code owners June 4, 2025 14:04
@bunnyshell Bunnyshell
Copy link
bunnyshell bot commented Jun 4, 2025 •
edited
Loading

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

  • 🔵 /bns:start to start the environment
  • 🚀 /bns:deploy to redeploy the environment
  • ❌ /bns:delete to remove the environment

@msoderberg msoderberg mentioned this pull request Jun 4, 2025
Open
@codecov Codecov
Copy link
codecov bot commented Jun 4, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 85.18519% with 4 lines in your changes missing coverage. Please review.

Project coverage is 59.98%. Comparing base (ee92cf5) to head (ca58149).

Files with missing lines Patch % Lines
util/settings/settings.go 78.94% 3 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #23260      +/-   ##
==========================================
+ Coverage   59.94%   59.98%   +0.04%     
==========================================
  Files         342      342              
  Lines       58605    58631      +26     
==========================================
+ Hits        35132    35172      +40     
+ Misses      20623    20617       -6     
+ Partials     2850     2842       -8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

agaudreault
agaudreault requested changes Jun 4, 2025
View reviewed changes
Copy link
Member
@agaudreault agaudreault left a comment

Choose a reason for hiding this comment

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

Can you add a video of the UI with some allowed labels? What does it looks like with ~10 labels?

8000
@msoderberg msoderberg force-pushed the feat/node-labels-pod-view branch from 6595dc7 to 4c1690a Compare June 4, 2025 19:14
@msoderberg
Copy link
Contributor Author

Thanks for the quick review! I've pushed an update.
Here are a couple of videos as well:

normal-use-case.mov
many-labels.mov

@msoderberg msoderberg force-pushed the feat/node-labels-pod-view branch from 4c1690a to ecccea2 Compare June 5, 2025 07:19
@msoderberg
Copy link
Contributor Author

When recording the "normal-use-case" yesterday I noticed that the keys overflowed. Those are quite common labels, and in general I think it make sense to give the key divs slightly more width than the value divs. Now it looks like this:
Screenshot 2025-06-05 at 09 27 47

@blakepettersson
Copy link
Member

@msoderberg I think also we need some docs, perhaps under UI customization?

@msoderberg
Copy link
Contributor Author

@msoderberg I think also we need some docs, perhaps under UI customization?

Good point 👍 I added a section to that page. WDYT?

@agaudreault
Copy link
Member

@linghaoSu What do you think of UI code and experience?

agaudreault
agaudreault requested changes Jun 5, 2025
View reviewed changes
agaudreault
agaudreault reviewed Jun 5, 2025
View reviewed changes
Copy link
Member
@agaudreault agaudreault left a comment

Choose a reason for hiding this comment

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

LGTM. Will wait for @linghaoSu or @keithchong to give some UI feedback

@agaudreault agaudreault added this to the v3.1 milestone Jun 9, 2025
linghaoSu
linghaoSu reviewed Jun 11, 2025
View reviewed changes
ui/src/app/applications/components/application-pod-view/pod-view.scss Outdated
@@ -76,11 +80,19 @@ $pod-age-icon-clr: #ffce25;
border-radius: 3px;
background-color: $argo-color-gray-3;
color: $argo-color-gray-6;
max-height: 100px;
overflow: auto;
div {
Copy link
Member

Choose a reason for hiding this comment

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

Using tag selectors in CSS is generally not recommended, as they can lead to unintended side effects and make styles harder to maintain.

It might be better to replace them with more semantic and specific selectors — such as class selectors — to improve clarity and avoid potential conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thank you!

@msoderberg
feat: optionally propagate node labels to application pod view
462177c 
Signed-off-by: Marcus Söderberg <msoderb@gmail.com>
@msoderberg
chore(ui/src/app/applications/components/application-pod-view/pod-vie…
0a77db5 
…w.tsx): lint-ui

Signed-off-by: Marcus Söderberg <msoderb@gmail.com>
@msoderberg
chore: fix typos; tweak error message; add unit test for GetAllowedNo…
c640360 
…deLabels

Signed-off-by: Marcus Söderberg <msoderb@gmail.com>
@msoderberg
fix(ui/src/app/applications/components/application-pod-view/pod-view.…
f1c4f6f 
…scss): update max-width ratio

Signed-off-by: Marcus Söderberg <msoderb@gmail.com>
@msoderberg
docs(docs/operator-manual/ui-customization.md): add section about nod…
4ae5a60 
…e labels in pod view

Signed-off-by: Marcus Söderberg <msoderb@gmail.com>
@msoderberg
changes from codegen
6a103bb 
Signed-off-by: Marcus Söderberg <msoderb@gmail.com>
@msoderberg msoderberg force-pushed the feat/node-labels-pod-view branch from 4fe3a63 to 6a103bb Compare June 12, 2025 06:34
@msoderberg @linghaoSu
chore: apply suggestions from code review
0c11de0 
Co-authored-by: Linghao Su <slh001@live.cn>
Signed-off-by: Marcus Söderberg <msoderb@gmail.com>
@msoderberg msoderberg force-pushed the feat/node-labels-pod-view branch from d4b1c83 to 0c11de0 Compare June 12, 2025 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agaudreault agaudreault agaudreault requested changes

@linghaoSu linghaoSu linghaoSu left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.1
Development

Successfully merging this pull request may close these issues.

Add labels to node information in the application pod view.
4 participants
@msoderberg @blakepettersson @agaudreault @linghaoSu
0