Skip to content

install goldctl in docker build #46640

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
fluttergithubbot merged 18 commits into from
Jan 10, 2020
Merged

install goldctl in docker build #46640

fluttergithubbot merged 18 commits into from
Jan 10, 2020

Conversation

@christopherfujino
Copy link
Contributor

@christopherfujino christopherfujino commented Dec 9, 2019 •
edited
Loading

Description

This moves the downloading and installing of goldctl to dev/ci/docker_linux/Dockerfile for the Cirrus linux tests. Previously, 10 Linux shards were running the install_goldctl.sh script as part of their "main" step. This speeds up running of tests and avoids occasional failure to download the tool, as seen in #44786.

Related Issues

Fixes #44786

Tests

I did not change any tests as my changes are exercised by the existing framework tests, which depend on goldctl being installed on the host to conduct goldens tests.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

@fluttergithubbot fluttergithubbot added c: contributor-productivity Team-specific productivity, code health, technical debt. work in progress; do not review labels Dec 9, 2019
@goderbauer
Copy link
Member

goderbauer commented Jan 7, 2020

Is this PR still relevant? Or should we close it?

@christopherfujino
Copy link
Contributor Author

christopherfujino commented Jan 8, 2020

@goderbauer yes, still relevant, I think I can land this soon :)

@christopherfujino christopherfujino marked this pull request as ready for review January 8, 2020 16:06
@Piinks
Copy link
Contributor

Piinks commented Jan 8, 2020

FYI @kjlubick

Piinks
Piinks approved these changes Jan 8, 2020
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

LGTM! This is great! Thanks!
I checked the Gold dashboard for results from this PR and all of the linux ones are accounted for (see here).

@Piinks
Copy link
Contributor

Piinks commented Jan 8, 2020

This would pin us at a specific version for Linux shards, but not Mac and Windows. My only concern is if having multiple versions across tests shards will create an issue, I would defer to @kjlubick.

@kjlubick
Copy link
Contributor

kjlubick commented Jan 9, 2020

goldctl doesn't change all that often and if it does, I try to backward compatible. I don't envision an issue, unless the Docker image is rarely re-built/updated.

@christopherfujino
Copy link
Contributor Author

christopherfujino commented Jan 9, 2020 •
edited
Loading

@kjlubick sounds good. For posterity's sake, if in the future there is a breaking change to goldctl that the Docker image has not picked up, a comment update PR to the Dockerfile will trigger a rebuild and pick up the latest goldctl.

@fluttergithubbot fluttergithubbot merged commit 7a13d8a into flutter:master Jan 10, 2020
@christopherfujino christopherfujino deleted the refactor-dockerfile branch January 10, 2020 17:51
@christopherfujino christopherfujino mentioned this pull request Jan 10, 2020
Closed
@dnfield dnfield mentioned this pull request Feb 4, 2020
Closed
dnfield added a commit that referenced this pull request Feb 4, 2020
@dnfield
Revert "install goldctl in docker build (#46640)"
0dc87f3 
This reverts commit 7a13d8a.
@dnfield dnfield mentioned this pull request Feb 4, 2020
Merged
dnfield added a commit that referenced this pull request Feb 4, 2020
@dnfield
Revert "install goldctl in docker build (#46640)" (#50088)
0299398 
This reverts commit 7a13d8a.
dnfield added a commit that referenced this pull request Feb 4, 2020
@dnfield
Revert "Revert "install goldctl in docker build (#46640)" (#50088)"
adaa4f6 
This reverts commit 0299398.
dnfield added a commit that referenced this pull request Feb 5, 2020
@dnfield
Reland "install goldctl in docker build" (#50124)
ed73f94 
* Revert "Revert "install goldctl in docker build (#46640)" (#50088)"

This reverts commit 0299398.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Piinks Piinks Piinks approved these changes

@gspencergoog gspencergoog Awaiting requested review from gspencergoog

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

download_goldctl.sh should retry if the depot_tools clone fails

6 participants

@christopherfujino @goderbauer @Piinks @kjlubick @googlebot @fluttergithubbot