Skip to content

Conversation

@CaseyHillers
Copy link
Contributor

@CaseyHillers CaseyHillers commented May 6, 2021

  • Migrate cocoon as an example

Issues

flutter/flutter#76140

.ci.yaml Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should postsubmit: false?

Copy link
Contributor Author

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)

Copy link
Contributor

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.

Copy link
Contributor Author

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)

Copy link
Contributor

@christopherfujino christopherfujino May 10, 2021

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.

Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants