Skip to content

[ci.yaml] Support branch regexes #1468

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Nov 29, 2021
Merged

[ci.yaml] Support branch regexes #1468

merged 6 commits into from
Nov 29, 2021

Conversation

@CaseyHillers
Copy link
Contributor

@CaseyHillers CaseyHillers commented Nov 16, 2021

This makes the release process easier. Instead of adding the candidate branch each time to the ci.yaml, we can insert the regex from this test and forget about the step.

Fixes flutter/flutter#84356

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read the [Flutter Style Guide] recently, and have followed its advice.
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@google-cla google-cla bot added the cla: yes label Nov 16, 2021
christopherfujino
christopherfujino reviewed Nov 16, 2021
View reviewed changes
CI_YAML.md Outdated
Copy link
Contributor

@christopherfujino christopherfujino Nov 16, 2021

Choose a reason for hiding this comment

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

the escaped backslashes make me sad

Copy link
Contributor

@christopherfujino christopherfujino Nov 16, 2021

Choose a reason for hiding this comment

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

Suggested change
- flutter-\\d+.\\d+-candidate.\\d+
- flutter-\\d+\.\\d+-candidate\.\\d+

Copy link
Contributor Author

@CaseyHillers CaseyHillers Nov 19, 2021

Choose a reason for hiding this comment

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

Added

Copy link
Contributor

@christopherfujino christopherfujino Nov 19, 2021

Choose a reason for hiding this comment

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

lol, my version was still wrong

christopherfujino
christopherfujino reviewed Nov 16, 2021
View reviewed changes
app_dart/lib/src/model/ci_yaml/ci_yaml.dart Outdated
Copy link
Contributor

@christopherfujino christopherfujino Nov 16, 2021

Choose a reason for hiding this comment

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

I suggest pulling this into a helper method, both to help debugging when inevitably we fail here, and so we can unit test it.

Copy link
Contributor

@christopherfujino christopherfujino Nov 16, 2021

Choose a reason for hiding this comment

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

also note turning 'master' into a regex means it will match 'my-feature-branch-off-of-master'

Copy link
Contributor Author

@CaseyHillers CaseyHillers Nov 18, 2021

Choose a reason for hiding this comment

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

Instead of adding this to the ci.yaml, I'll add it in the validation here. It'll assume this is supposed to be a full line match (so prefix with $ and suffix with ^)

godofredoc
godofredoc reviewed Nov 16, 2021
View reviewed changes
app_dart/test/service/scheduler_test.dart Outdated
Copy link
Contributor

@godofredoc godofredoc Nov 16, 2021

Choose a reason for hiding this comment

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

Should this also be added to the integration tests? free form regular expressions are error prone.

Copy link
Contributor Author

@CaseyHillers CaseyHillers Nov 18, 2021

Choose a reason for hiding this comment

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

Added! However, it relies on their being a git executable on path. For now, I'll leave this in the Cocoon tree.

I sent flutter/engine#29820 to get the test passing

Copy link
Contributor

@christopherfujino christopherfujino Nov 19, 2021

Choose a reason for hiding this comment

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

I see, so we have to land the engine PR first?

Copy link
Contributor Author

@CaseyHillers CaseyHillers Nov 19, 2021

Choose a reason for hiding this comment

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

Yes

christopherfujino
christopherfujino approved these changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM, obviously pending CI going green.

I also approved the engine PR

@CaseyHillers CaseyHillers added the waiting for tree to go green Merge PR when tree becomes green via fluttergithubbot label Nov 19, 2021
@fluttergithubbot
Copy link

fluttergithubbot commented Nov 19, 2021

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Linux Cocoon has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • This commit has empty status or empty checks. Please check the Google CLA status is present and Flutter Dashboard application has multiple checks.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green Merge PR when tree becomes green via fluttergithubbot label Nov 19, 2021
@CaseyHillers CaseyHillers added the waiting for tree to go green Merge PR when tree becomes green via fluttergithubbot label Nov 24, 2021
@fluttergithubbot
Copy link

fluttergithubbot commented Nov 24, 2021

This pull request is not suitable for automatic merging in its current state.

  • This commit is not mergeable and has conflicts. Please rebase your PR and fix all the conflicts.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green Merge PR when tree becomes green via fluttergithubbot label Nov 24, 2021
@godofredoc
Copy link
Contributor

godofredoc commented Nov 24, 2021

Should we wait to land this change until monday?

@CaseyHillers
Copy link
Contributor Author

CaseyHillers commented Nov 24, 2021

That's fair. I thought we froze the cloud build deploys. I'll land this Monday

@CaseyHillers CaseyHillers added the waiting for tree to go green Merge PR when tree becomes green via fluttergithubbot label Nov 29, 2021
@fluttergithubbot fluttergithubbot merged commit f96fafb into flutter:main Nov 29, 2021
@CaseyHillers CaseyHillers deleted the ci-yaml-branch-regex branch December 9, 2021 21:49
This was referenced Jan 11, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@godofredoc godofredoc Awaiting requested review from godofredoc

1 more reviewer

@christopherfujino christopherfujino christopherfujino approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

waiting for tree to go green Merge PR when tree becomes green via fluttergithubbot

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[scheduler] Allow regexes in ci.yaml enabled_branches

4 participants

@CaseyHillers @fluttergithubbot @godofredoc @christopherfujino