Skip to content
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

flutter/packages needs a LUCI solution for check overrides #130076

Closed
stuartmorgan opened this issue Jul 6, 2023 · 15 comments
Closed

flutter/packages needs a LUCI solution for check overrides #130076

stuartmorgan opened this issue Jul 6, 2023 · 15 comments
Assignees
Labels
P1 High-priority issues at the top of the work list package flutter/packages repository. See also p: labels. team-infra Owned by Infrastructure team

Comments

@stuartmorgan
Copy link
Contributor

The version-check command in the flutter/packages CI is set up to, in presubmit, look for likely-missing version changes, as well as for major version changes to platform interface packages (since we almost never actually want to do those). Because it can have false positives, we need a way to override it when it's wrong—the idea is that this is something that we frequently forget (missing versions) or someone might not realize is a big deal (major version bumps to platform interfaces), so we want something that requires a human to explicitly say "yes, we looked at this, and we deliberately chose not to do what we are doing, rather than forgetting or not realizing".

How can we handle this in LUCI tests?

(The way it works with the Cirrus config is that one of the special env variables Cirrus passes to all runs is CIRRUS_PR_LABELS, which has the current set of GitHub labels on the PR, and so we set a label to indicate that we want to override, re-run that check, and the tooling knows to then not fail. We don't need it to be that specific flow, but we need some solution.)

@stuartmorgan stuartmorgan added package flutter/packages repository. See also p: labels. team-infra Owned by Infrastructure team labels Jul 6, 2023
@stuartmorgan stuartmorgan added the P1 High-priority issues at the top of the work list label Jul 6, 2023
@flutter-triage-bot flutter-triage-bot bot added team-ecosystem Owned by Ecosystem team triaged-ecosystem Triaged by Ecosystem team and removed team-infra Owned by Infrastructure team team-ecosystem Owned by Ecosystem team triaged-ecosystem Triaged by Ecosystem team labels Jul 8, 2023
@flutter-triage-bot
Copy link

Issue is assigned to multiple teams (infra, ecosystem). Please ensure the issue has only one team-* label at a time. Use fyi-* labels to have another team look at the issue without reassigning it.
The triaged-ecosystem label is irrelevant if there is no team-ecosystem label or fyi-ecosystem label.

@stuartmorgan stuartmorgan added the team-infra Owned by Infrastructure team label Jul 8, 2023
@yusuf-goog yusuf-goog self-assigned this Jul 13, 2023
@yusuf-goog
Copy link
Contributor

I'm checking on this, but I don't think we have a way of passing information from the PR to the builds.

Would the characteristics of this requirement be satisfied by having a value set in the .ci.yaml file?

@stuartmorgan
Copy link
Contributor Author

Would the characteristics of this requirement be satisfied by having a value set in the .ci.yaml file?

I'm not following; how would we override a check for one specific PR with .ci.yaml?

@stuartmorgan
Copy link
Contributor Author

This is now the last dependency on Cirrus in flutter/packages.

yusuf-goog added a commit to yusuf-goog/cocoon that referenced this issue Aug 28, 2023
yusuf-goog added a commit to yusuf-goog/cocoon that referenced this issue Aug 28, 2023
yusuf-goog added a commit to yusuf-goog/cocoon that referenced this issue Aug 28, 2023
yusuf-goog added a commit to yusuf-goog/cocoon that referenced this issue Aug 28, 2023
yusuf-goog added a commit to yusuf-goog/cocoon that referenced this issue Aug 28, 2023
yusuf-goog added a commit to yusuf-goog/cocoon that referenced this issue Aug 29, 2023
yusuf-goog added a commit to yusuf-goog/cocoon that referenced this issue Aug 29, 2023
yusuf-goog added a commit to yusuf-goog/cocoon that referenced this issue Aug 29, 2023
yusuf-goog added a commit to yusuf-goog/cocoon that referenced this issue Aug 29, 2023
yusuf-goog added a commit to yusuf-goog/cocoon that referenced this issue Aug 29, 2023
yusuf-goog added a commit to yusuf-goog/cocoon that referenced this issue Aug 29, 2023
auto-submit bot pushed a commit to flutter/cocoon that referenced this issue Aug 29, 2023
Bug:flutter/flutter#130076

*Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.*

*List which issues are fixed by this PR. You must list at least one issue.*

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*
@yusuf-goog
Copy link
Contributor

@stuartmorgan change is in to propagate the labels to build properties, which you can use in your recipes.

@stuartmorgan
Copy link
Contributor Author

which you can use in your recipes.

We just have the one recipe; could you update it to transfer that property to an environment variable? That would let us directly migrate the existing task.

@yusuf-goog
Copy link
Contributor

Yes, I'll do that

@yusuf-goog
Copy link
Contributor

@yusuf-goog
Copy link
Contributor

Change submitted. closing.

@stuartmorgan
Copy link
Contributor Author

This does not appear to be working. I made a test PR, verified that it failed as expected, then added an override label and re-ran the test. On the re-run, there environment variable is empty:
https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8771239673077318481/+/u/Run_package_tests/CHANGELOG_and_version_validation/execution_details

@stuartmorgan stuartmorgan reopened this Aug 31, 2023
@yusuf-goog
Copy link
Contributor

I think my change hasn't been pushed to prod yet, so its not appearing. tracking down the deployment.

@yusuf-goog
Copy link
Contributor

Looks like flutter/cocoon#3032 blocking deployment of changes.

@stuartmorgan
Copy link
Contributor Author

Thanks for tracking that down; I saw the variable being there and assumed that things had propagated, and didn't consider that just the recipe part could have.

Let me know when it's good to re-test!

@yusuf-goog
Copy link
Contributor

auto-submit bot pushed a commit to flutter/packages that referenced this issue Aug 31, 2023
Adds a LUCI version of the version presubmit check, using the new label passthrough functionality, and removes it from Cirrus. Since that was the last Cirrus task, this removes the entire Cirrus config, and the associated Dockerfile.

Part of flutter/flutter#130076
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P1 High-priority issues at the top of the work list package flutter/packages repository. See also p: labels. team-infra Owned by Infrastructure team
Projects
None yet
Development

No branches or pull requests

2 participants