-
Notifications
You must be signed in to change notification settings - Fork 28
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
WD-8401 - Remove secrets #1704
WD-8401 - Remove secrets #1704
Conversation
Demo starting at https://juju-dashboard-1704.demos.haus |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1704 +/- ##
==========================================
+ Coverage 95.44% 95.46% +0.01%
==========================================
Files 187 189 +2
Lines 5596 5686 +90
Branches 1616 1638 +22
==========================================
+ Hits 5341 5428 +87
- Misses 235 238 +3
Partials 20 20 ☔ View full report in Codecov by Sentry. |
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.
Looks good and works as expected locally (apart from the backend bug related to removing the last revision) 🚀
label={Label.REMOVE_ALL} | ||
name="removeAll" | ||
type="checkbox" | ||
disabled={secret?.revisions.length === 1} |
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.
I was wondering if maybe we could leave it always enabled? In the case when there is only 1 revision, we are still removing all the revisions.
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.
I was thinking it could be confusing if the checkbox was enabled as it's giving you a choice and the user might wonder if there will be a different outcome depending on which option they choose. I wonder if there is only one revision we should hide both fields and display some text instead? Maybe something like: "This secret has one revision (7) and will be completely removed."
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.
That's a good idea! If it is not that lengthy to implement, I think it might improve the user experience if done.
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.
Done.
62c4128
to
02e6af7
Compare
I'm going to land this even though this bug exists, as this is not the only place this bug affects us, and once we have a solution we can update all the places (if it doesn't get rectified on the backend). |
Done
QA
juju update-secret <URI> token=34ae35facd4
(you can run the same command a few times and each time a new revision will get created).Details
https://warthogs.atlassian.net/browse/WD-8401