-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
feat(tasks): allow skipping tasks with the flag #1561
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1561 +/- ##
==========================================
+ Coverage 97.28% 97.29% +0.01%
==========================================
Files 48 48
Lines 4603 4621 +18
==========================================
+ Hits 4478 4496 +18
Misses 125 125
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ 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 solid 🙂
Do I understand correctly that passing --skip-tasks
means I don't have to also pass --trust
?
No, you still have to pass |
OK yes that makes sense. |
65c1f9e
to
7e32e3a
Compare
7e32e3a
to
104c2bf
Compare
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.
Two suggestions regarding readability and then I'll have nothing left to say I think 😄
104c2bf
to
c4f606f
Compare
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!
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 also need to document this setting here.
c4f606f
to
ff20642
Compare
PR updated with the setting documentation 👍🏼 |
Co-authored-by: Timothée Mazzucotelli <[email protected]>
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.
Just some final 🤞 docs suggestions, @noirbizarre. I'll be happy to merge afterwards.
Co-authored-by: Sigurd Spieckermann <[email protected]>
Co-authored-by: Sigurd Spieckermann <[email protected]>
Co-authored-by: Sigurd Spieckermann <[email protected]>
Co-authored-by: Sigurd Spieckermann <[email protected]>
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.
Fantastic, thanks @noirbizarre! 🙏
Sorry for being late to the party. I think that, if you pass this flag, it should be taken into account around here to avoid marking the template as unsafe if it's only because of tasks but you're skipping them: Lines 233 to 234 in 70aa69f
|
@yajo I think that's the same thought that I had, but we decided against it in that discussion. WDYT? |
Consider migrations. Even if the template uses migrations, we only mark it as unsafe if the current update will trigger them. In the same fashion, if it has tasks but you're ignoring them, there's no unsafety produced by tasks. Other unsafety criteria should still be honored, but not that one. |
This PR add a
-T/--skip-tasks
flag allowing to skip template tasks execution.Some known use cases I have:
Should fix #1415