Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

add supported luci builders #20099

Merged
merged 8 commits into from
Aug 4, 2020
Merged

Conversation

keyonghan
Copy link
Contributor

@keyonghan keyonghan commented Jul 28, 2020

Description

Existing Luci builder configurations are stored in cocoon project. This is not efficient because we need to re-deploy the flutter-dashboard after changing the builder configuration. Additionally, with different channels/branches, a single place becomes challenging to support various requests.

Moving build configuration files to engine repo by itself will make things easier. Repos/branches will maintain their own copy of builder config file. From cocoon side, it can directly obtain the build config file for different repos/branches without redeploying the app engine.

JSON file will be validated via luci presubmit test, to be defined in engine.py recipe.

Related Issues

Related issue: flutter/flutter#62429, flutter/flutter#53371

@keyonghan keyonghan requested a review from godofredoc July 28, 2020 23:09
@auto-assign auto-assign bot requested a review from liyuqian July 28, 2020 23:10
@godofredoc
Copy link
Contributor

Can we update the description with more information why we are moving these file to the engine repo?

@godofredoc godofredoc requested a review from dnfield July 29, 2020 00:35
@godofredoc
Copy link
Contributor

Can you also please add a README file describing the expected format of the json file and a test?

@keyonghan
Copy link
Contributor Author

Can you also please add a README file describing the expected format of the json file and a test?

README.md added. As for test, please find comment: flutter/cocoon#875 (comment)

@keyonghan
Copy link
Contributor Author

Can we update the description with more information why we are moving these file to the engine repo?

Updated.

@keyonghan keyonghan force-pushed the add_luci_builders branch from de4bf84 to 9b257bd Compare July 31, 2020 21:02
@keyonghan
Copy link
Contributor Author

keyonghan commented Jul 31, 2020

@dnfield @liyuqian could you help with the following questions?

  • what is the best dir location to save luci config builders as a json file?
  • Is there already a test coverage to check validity of a json file in engine? I see there are quite some json files in engine repo, how are their validity tested?
  • If we add some new file, anything else to run/add/check to have tests, like build_test, to pass? I added a dart file, but it hit license error.
  • Is there any guidelines/doc for new contributors to follow w.r.t. engine repo?

Thank you!

@liyuqian
Copy link
Contributor

liyuqian commented Aug 3, 2020

@dnfield @liyuqian could you help with the following questions?

  • what is the best dir location to save luci config builders as a json file?

Somewhere in ci subdirectory.

  • Is there already a test coverage to check validity of a json file in engine? I see there are quite some json files in engine repo, how are their validity tested?

Not that I'm aware of. But it should be simple to add as a Cirrus or LUCI test by checking the json from a Dart script.

  • If we add some new file, anything else to run/add/check to have tests, like build_test, to pass? I added a dart file, but it hit license error.

For license errors, we often just copy the diffs from the presubmit test to the corresponding local files to make it pass. Reproducing and checking license errors locally was difficult according to my last experience so I just let presubmit tests check if the change is good.

  • Is there any guidelines/doc for new contributors to follow w.r.t. engine repo?

See "Engine repo" section in https://github.com/flutter/flutter/wiki

@keyonghan
Copy link
Contributor Author

Thank you @liyuqian !

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@@ -0,0 +1,76 @@
// Copyright 2020 The Flutter Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR but given this validation will be used across repos I'm wondering if it could be moved to the recipes to avoid duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point! moving the validation to a module should help. Will take a look.

@keyonghan keyonghan deleted the add_luci_builders branch August 31, 2023 20:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants