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
Merged
merged 9 commits into from
Jun 13, 2025
Merged

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

merged 9 commits into from
Jun 13, 2025

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
Copy link

bunnyshell bot commented Jun 4, 2025 •
edited
Loading

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@msoderberg msoderberg mentioned this pull request Jun 4, 2025
Closed
@codecov
Copy link

codecov bot commented Jun 4, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 85.18519% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.97%. Comparing base (ee92cf5) to head (3af1c89).
⚠️ Report is 429 commits behind head on master.

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.97%   +0.02%     
==========================================
  Files         342      342              
  Lines       58605    58645      +40     
==========================================
+ Hits        35132    35172      +40     
- Misses      20623    20627       +4     
+ Partials     2850     2846       -4

☔ 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?

@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

msoderberg commented Jun 4, 2025

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

msoderberg commented Jun 5, 2025

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

blakepettersson commented Jun 5, 2025

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

@msoderberg
Copy link
Contributor Author

msoderberg commented Jun 5, 2025

@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

agaudreault commented Jun 5, 2025

@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
color: $argo-color-gray-6;
max-height: 100px;
overflow: auto;
div {
Copy link
Member

@linghaoSu linghaoSu Jun 11, 2025

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

@msoderberg msoderberg Jun 12, 2025

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 <[email protected]>
@msoderberg
chore(ui/src/app/applications/components/application-pod-view/pod-vie…
0a77db5 
…w.tsx): lint-ui

Signed-off-by: Marcus Söderberg <[email protected]>
@msoderberg
chore: fix typos; tweak error message; add unit test for GetAllowedNo…
c640360 
…deLabels

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

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

Signed-off-by: Marcus Söderberg <[email protected]>
@msoderberg
changes from codegen
6a103bb 
Signed-off-by: Marcus Söderberg <[email protected]>
@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 <[email protected]>
Signed-off-by: Marcus Söderberg <[email protected]>
@msoderberg msoderberg force-pushed the feat/node-labels-pod-view branch from d4b1c83 to 0c11de0 Compare June 12, 2025 06:38
@msoderberg @linghaoSu
chore(ui/src/app/applications/components/application-pod-view/pod-vie…
3af1c89 
…w.scss): apply suggestion from code review

Co-authored-by: Linghao Su <[email protected]>
Signed-off-by: Marcus Söderberg <[email protected]>
@agaudreault agaudreault merged commit 00ee32f into argoproj:master Jun 13, 2025
41 of 43 checks passed
dsuhinin pushed a commit to dsuhinin/argo-cd that referenced this pull request Jun 16, 2025
@msoderberg @linghaoSu @dsuhinin
feat: optionally propagate node labels to application pod view (argop…
6ab5799 
…roj#15274) (argoproj#23260)

Signed-off-by: Marcus Söderberg <[email protected]>
Co-authored-by: Linghao Su <[email protected]>
Signed-off-by: dsuhinin <[email protected]>
jcogilvie pushed a commit to Sanyaku/argo-cd that referenced this pull request Jun 16, 2025
@msoderberg @linghaoSu @jcogilvie
feat: optionally propagate node labels to application pod view (argop…
3cd2a6a 
…roj#15274) (argoproj#23260)

Signed-off-by: Marcus Söderberg <[email protected]>
Co-authored-by: Linghao Su <[email protected]>
Signed-off-by: Jonathan Ogilvie <[email protected]>
enneitex pushed a commit to enneitex/argo-cd that referenced this pull request Aug 24, 2025
@msoderberg @linghaoSu @enneitex
feat: optionally propagate node labels to application pod view (argop…
fba03e5 
…roj#15274) (argoproj#23260)

Signed-off-by: Marcus Söderberg <[email protected]>
Co-authored-by: Linghao Su <[email protected]>
Signed-off-by: enneitex <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agaudreault agaudreault agaudreault approved these changes

+1 more reviewer

@linghaoSu linghaoSu linghaoSu left review comments

Reviewers whose approvals may not affect merge requirements

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