Skip to content

Conversation

@yusuf-goog
Copy link
Contributor

@yusuf-goog yusuf-goog commented May 3, 2022

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

  • 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 and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • 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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

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
@yusuf-goog yusuf-goog requested a review from keyonghan as a code owner May 3, 2022 23:55
@yusuf-goog yusuf-goog requested a review from CaseyHillers May 3, 2022 23:55
.ci.yaml Outdated
- flutter-\d+\.\d+-candidate\.\d+

platform_properties:
linux_staging_build:
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor

@keyonghan keyonghan May 4, 2022

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@keyonghan keyonghan May 4, 2022

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@yusuf-goog
Copy link
Contributor Author

yusuf-goog commented May 4, 2022 via email

@yusuf-goog yusuf-goog requested a review from godofredoc May 4, 2022 16:04
yusuf-goog added 2 commits May 5, 2022 10:44
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:
Copy link
Contributor

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.

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.

3 participants