-
Notifications
You must be signed in to change notification settings - Fork 100
[scheduler] ci migration script #1202
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
cc239a1 to
203f1f7
Compare
.ci.yaml
Outdated
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 postsubmit: false?
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.
Is it being implicit ok? The proto defaults to false for presubmit and postsubmit, and I prefer that as it makes it easier to skim the config as existence of presubmit or postsubmit indicates the target runs on that (albeit you could do postsubmit: false)
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 would prefer that either the implicit default be documented in the protobuf, or the config be explicit. The goal being that someone can refer only to the config file and the protobuf and understand what is being built and with what properties.
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 protos did not default to false, updated and added to documentation)
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.
change this to double quotes and no escaped single quote, I believe an upcoming lint will enforce. I know this is copy pasted from my code, I prefer it this way, but the lints have spoken.
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, thanks for being proactive on the lint!
409960f to
0308954
Compare
0308954 to
289ba82
Compare
Issues
flutter/flutter#76140