-
Notifications
You must be signed in to change notification settings - Fork 60
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
Improve snapshot actions across devices, instances and applications #3885
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3885 +/- ##
=======================================
Coverage 79.28% 79.28%
=======================================
Files 281 281
Lines 12754 12755 +1
Branches 2844 2844
=======================================
+ Hits 10112 10113 +1
Misses 2642 2642
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Also makes existing tests less brittle when re-ran
…lowfuse into 3815-snapshot-actions
also, ensure downloads dir is cleared before test
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.
partial review, unable to leave more comments for it due to This diff is outdated, please refresh and try again.
kind: 'danger', | ||
confirmLabel: 'Delete' | ||
}, async () => { | ||
await SnapshotsApi.deleteSnapshot(snapshot.id) |
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 we treat possible failures?
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.
Sorry, not sure what the question is here
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.
Was wondering if we should threat the possibility for the deleteSnapshot endpoint to fail
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.
haven't went over the cypress tests yet, would love to have a chat before starting to make changes
@cstns all suggestions we discussed now in place and working and tests passing again. Ready for (re)review. |
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 on my end! We can move forward even without the modal validation when deleting snapshots because i couldn't find any other delete dialogs that implement such a catch
kind: 'danger', | ||
confirmLabel: 'Delete' | ||
}, async () => { | ||
await SnapshotsApi.deleteSnapshot(snapshot.id) |
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.
Was wondering if we should threat the possibility for the deleteSnapshot endpoint to fail
closes #3815
closes #3629
Description
Caveats
Some menu items do not make a sense on other views and thus not all menus are the same. This was discussed and agreed in the issue comment
TODO:
Related Issue(s)
Checklist
flowforge.yml
?FlowFuse/helm
to update ConfigMap TemplateFlowFuse/CloudProject
to update values for Staging/ProductionLabels
area:migration
label