Skip to content

Fix Gold flake from gsutil fallback#50149

Merged
fluttergithubbot merged 12 commits intoflutter:masterfrom
Piinks:goldFlake
Feb 5, 2020
Merged

Fix Gold flake from gsutil fallback#50149
fluttergithubbot merged 12 commits intoflutter:masterfrom
Piinks:goldFlake

Conversation

@Piinks
Copy link
Contributor

@Piinks Piinks commented Feb 4, 2020 •
edited
Loading

Description

This fixes the flake in #49917
The flake is caused by the fact that, for some PRs, an empty auth method is used in presubmit testing for contributors that cannot decrypt the service account. This empty auth method still creates an authorization file, although it is empty. This file was cached after the fact in a randomly generated folder. Later on, another PR may randomly choose the same directory and need authentication for tryjobs, but thinks it's already authenticated. 💣 💥

Additional re-factoring is already scheduled here to support Luci testing, with the eventual goal of removing the need for a service account at all. That work is being tracked by #49837 and #49749.

Related Issues

Fixes #49917
Fixes #50148

Tests

Client will re-authorize if gsutil is enabled

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.

@Piinks Piinks added a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. team: flakes team-infra Owned by Infrastructure team labels Feb 4, 2020
Piinks
Piinks commented Feb 4, 2020
View reviewed changes
packages/flutter/lib/src/animation/animation_controller.dart
/// animation.
///
/// The new value is clamped to the range set by [lowerBound] and [upperBound].
/// The new value is clamped to the range set by [lowerBound] and
Copy link
Contributor Author

@Piinks Piinks Feb 4, 2020

Choose a reason for hiding this comment

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

Trivial >80 character change to trigger framework tests.

@Piinks Piinks requested a review from dnfield February 4, 2020 23:08
dnfield
dnfield reviewed Feb 4, 2020
View reviewed changes
dnfield
dnfield approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM with TODO

dnfield
dnfield reviewed Feb 5, 2020
View reviewed changes
packages/flutter_goldens/lib/flutter_goldens.dart Outdated
defaultComparator,
platform,
suffix: '${math.Random().nextInt(10000)}',
onCirrus: platform.environment.containsKey('CIRRUS_CI'),
Copy link
Contributor

@dnfield dnfield Feb 5, 2020

Choose a reason for hiding this comment

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

This should work but we could run into similar problems on LUCI if we're caching anything there - we're not right now but we might in the future.

Should we just make it opt in to put it in the pkg folder so that local users can do that?

Copy link
Contributor Author

@Piinks Piinks Feb 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh this will be different when I refactor for Luci. Do you mean we should default to using systemTempDirectory and set the flag for the other way around, to use the pkg folder if we are local?

Copy link
Contributor

@dnfield dnfield Feb 5, 2020

Choose a reason for hiding this comment

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

Yeah, although if we have to change this anyway for LUCI it's probably not a big deal either way.

@Piinks
Copy link
Contributor Author

Piinks commented Feb 5, 2020

Ok, I think using the Directory.systemTemp for Cirrus will keep files out of the cache, PTAL.

Is there a way that I can invalidate the current pkg cache so it will take out the trash? Or will it eventually clean itself out?

@dnfield
Copy link
Contributor

dnfield commented Feb 5, 2020

@fkorotkov may know how to kill a cache - I'm not aware of a way.

@fkorotkov
Copy link
Contributor

fkorotkov commented Feb 5, 2020

Cirrus re-uploads cache if it changes. BTW if you want different caches for collaborators vs no you can add echo $CIRRUS_USER_COLLABORATOR to the fingerprint_script.

@dnfield
Copy link
Contributor

dnfield commented Feb 5, 2020

@fkorotkov - we've unintentionally introduced files into our caches that we don't want. Can we just clobber them all?

@fkorotkov
Copy link
Contributor

fkorotkov commented Feb 5, 2020

I can kill flutter_pkg-b9000965872fccdeaecb549276f69fd8bbec04bc863f0ee4700f6d10b5b0b510 manually. There is no UI to invalidate caches (cirruslabs/cirrus-ci-docs#379) at the moment unfortunately.

@dnfield
Copy link
Contributor

dnfield commented Feb 5, 2020

Thanks!

@Piinks
Copy link
Contributor Author

Piinks commented Feb 5, 2020

I can kill flutter_pkg-b9000965872fccdeaecb549276f69fd8bbec04bc863f0ee4700f6d10b5b0b510 manually. There is no UI to invalidate caches (cirruslabs/cirrus-ci-docs#379) at the moment unfortunately.

Thank you!
We should wait to nuke it until this is fixed. Otherwise it will just continue to fill up and we'll have to nuke it again.

@dnfield
Copy link
Contributor

dnfield commented Feb 5, 2020

We could also just update the fingerprint script (e.g. just add some string to the end of the output) in this pull so that future pulls will use a different cache.

@fkorotkov
Copy link
Contributor

fkorotkov commented Feb 5, 2020

nuked it 🙌

Piinks
Piinks commented Feb 5, 2020
View reviewed changes
@Piinks
Copy link
Contributor Author

Piinks commented Feb 5, 2020

I made a few teaks @dnfield, should be ready to go now. :)

@fluttergithubbot fluttergithubbot merged commit 1c15cd8 into flutter:master Feb 5, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
@Piinks Piinks deleted the goldFlake branch March 14, 2025 17:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@dnfield dnfield dnfield approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. team-infra Owned by Infrastructure team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Gold artifacts are being cached on Cirrus that shouldn't be Gold flake: user does not have permission to list objects

5 participants

@Piinks @dnfield @fkorotkov @googlebot @fluttergithubbot