-
Notifications
You must be signed in to change notification settings - Fork 2
Update API response to include status.lastAppliedRevision fields #1643
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Can we add a test for the new field to make sure it's coming back as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No blockers
@@ -866,10 +866,6 @@ exports[`Applications index test snapshots success 1`] = ` | |||
text-overflow: ellipsis; | |||
} | |||
|
|||
.c15 .filter-options-chip { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens a lot lately, when running npm test -- -u
without even change a thing in application
@@ -1,3 +1,4 @@ | |||
/* eslint-disable testing-library/no-node-access */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Any idea why we would want to do DOM traversal here (had to look up what this rule did). https://github.com/testing-library/eslint-plugin-testing-library/blob/main/docs/rules/no-node-access.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to select an element from another element here and testing-library
doesn't support all the functionality we want.
Closes #1606
What changed?
Set
lastAppliedRevision
to every workloadWhy was this change made?
Show requested and deployed version of a Helm Release within Pipelines details
Feedback from CX that:
Only showing the declared version makes it difficult to understand state when using semver range
Showing both the HelmRelease spec.chart.spec.version and status.lastAppliedRevision would help with users quickly trying to eyeball the state of their application rollout