-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Adding two staging tests to ci.yaml #103003
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
Adding a properties block (linux_stating_build) and 2 builds that use this to ci.yaml. This is to verify some failures on these builds that are happening when running via entries in devicelab_stating_config.star file. Bug:102406
.ci.yaml
Outdated
| - flutter-\d+\.\d+-candidate\.\d+ | ||
|
|
||
| platform_properties: | ||
| linux_staging_build: |
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 am a bit reluctant to add this set of builders in the .ci.yaml.
Instead, after taking a look at the failure: https://ci.chromium.org/ui/p/flutter/builders/staging/Linux_staging%20analyze/62/overview
you are missing the validation property: https://github.com/flutter/flutter/blob/master/.ci.yaml#L222-L223
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.
Shouldn't we be moving to this scheme and deprecating running staging tests with devicelab_staging_config.star, to avoid having to maintain 2 different methods of running staging tests, and avoid issues like this where config is different?
If we choose to keep devicelab_staging_config.star as a way of testing staging tests, we will continue to experience inconsistencies and overhead with our main ci.yaml based mechanism of defining builds.
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.
+1 to moving it into the ci.yaml. I think some ways to simplify it is we could update the ci.yaml infra to instead not require an explicit staging platform_properties, but other than that this approach 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.
How do we validate all Linux builders (debian->ubuntu) work without affecting our end users? With this, we need to add ~100 new entries to the dashboard and then remove all of them after.
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.
Your correct, right now, the view is going to be messy when we do this. And we should probably find a more graceful way of handling the build view affects in the future (maybe hide flaky by default, or even have a staging:true property that hides only the staging ones).
However, I think utilizing this same mechanism thats used for prod builds to schedule these staging ones are good to do longterm.
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.
That sounds fair. The ideal case is we can easily control what targets to be enabled in prod (show up in dashboard), what targets to to enabled in bringup: true (show up in dashboard), and what targets to be enabled in staging for validation purpose only (hide from dashboard), using a single source of truth of the .ci.yaml.
But these need backend refactoring to support. Before we have a clear plan/support, I recommend use existing staging star to validate.
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 still think the easiest way would be to add os: Ubuntu to targets we want to individually migrate, and do it in batches. Once everything is swapped, we can update the platform_properties to default to ubuntu.
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.
Switching it that way doesn't allow provide the ability to let them soak for a while. Having the ability to setup staging jobs and let them run as we need would be very helpful. Even if we have to do them in blocks.
|
it would be cool if we could inherit properties, so the staging properties
would only be the delta to the prod properties.
…On Tue, May 3, 2022 at 8:15 PM Casey Hillers ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In .ci.yaml
<#103003 (comment)>:
> @@ -10,6 +10,23 @@ enabled_branches:
- flutter-\d+\.\d+-candidate\.\d+
platform_properties:
+ linux_staging_build:
+1 to moving it into the ci.yaml. I think some ways to simplify it is we
could update the ci.yaml infra to instead not require an explicit staging
platform_properties, but other than that this approach LGTM
—
Reply to this email directly, view it on GitHub
<#103003 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AV3Q2C4BR6OLJZFX52GLHHDVIHTUPANCNFSM5VANEOHQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Staging builds now use a name prefix of Staging_build_, which allows us to filter these builds on the dashboard.
We can't add bringup:true to properties block level, so need to move this back to each target.
| - flutter-\d+\.\d+-candidate\.\d+ | ||
|
|
||
| platform_properties: | ||
| staging_build_linux: |
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.
Synced up offline with Yusuf about this. Using staging_build_linux makes more sense the Linux_staging as there's quite a bit of infra and dashboarding that would need to change to support the latter. So doing this is quicker.
Adding a properties block (linux_stating_build) and 2 builds
that use this to ci.yaml. This is to verify some failures on
these builds that are happening when running via entries
in devicelab_stating_config.star file.
Bug:102406
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.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.