-
Notifications
You must be signed in to change notification settings - Fork 91
feat(storage-box): implement actions #1145
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## storage-boxes #1145 +/- ##
================================================
Coverage ? 64.28%
================================================
Files ? 267
Lines ? 11743
Branches ? 0
================================================
Hits ? 7549
Misses ? 3380
Partials ? 814
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
63258d9 to
b54efad
Compare
c58e759 to
b99852c
Compare
|
|
||
| _ = cmd.RegisterFlagCompletionFunc("day-of-week", cmpl.SuggestCandidates("monday", "tuesday", "wednesday", "thursday", "friday", "saturday", "sunday")) | ||
|
|
||
| // TODO: should we add some validation here to avoid footguns? (e.g. backing up every minute) |
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.
The frontend does not allow unsetting the values I think. What about having -1 for every minute/hour, and we default to UTC 00:00?
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.
If we want the same behavior as the frontend, we could also just require a value for minute and hour
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.
And not allow snapshotting every minute, or every hour at all? For snapshotting every minute, I would be okay not allowing it, because I can't imagine some needing that. For snapshotting every hour, I am not sure, but probably no wants that?
If you want to move forward, I would be okay with making this a TODO in the code and marking both as required for now.
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 think snapshotting every hour would definitely make sense, but the frontend requires both an hour and minute 🤷
If we allow not setting it, then the user could cause invalid state in the frontend using the CLI.
Ideally I think defaulting to UTC 00:00 would make the most sense. Otherwise when a user for example only sets --day-of-week, a snapshot would be created at every minute of that day of the week. Creating only one snapshot on that day at 00:00 would definitely make more sense.
On the other hand, if we allow -1 as a value, we deviate from the API. So I'm not sure what what would be the best proceeding here. If we default to 00:00 and allow -1 for minute or hour, the behavior is different in the API, CLI and the frontend.
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.
If think if we support every hour, we should definitely support every minute.
I can think of:
--minute * --hour * --day-of-week sundaywould be more intuitive I guess, but harder to parse--every-minute --every-hour --day-of-week sundayadds more flags, but would be very specific about the behaviour--minute -1 --hour -1 --day-of-weeknot as intuitive as*, but easier to parse
I think I'd prefer either the first or second option. I think we should discuss this at least with @jooola or @apricote.
But as said, we can also merge this now and talk about this internally.
|
@phm07 Looks good to me. Can you do a rebase and address the open issues, then I would take a final look, and we can merge this in |
b99852c to
05160ac
Compare
This PR implements actions for Storage Boxes.
This PR implements actions for Storage Boxes.
This PR implements actions for Storage Boxes.
This PR implements actions for Storage Boxes.
This PR implements actions for Storage Boxes.
This PR implements actions for Storage Boxes.
This PR implements actions for Storage Boxes.
This PR implements actions for Storage Boxes.
This PR implements actions for Storage Boxes.
This PR implements actions for Storage Boxes.