Skip to content
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

🏗 Persist merge commit for reuse with later CI jobs #33165

Merged
merged 13 commits into from
Mar 10, 2021

Conversation

jridgewell
Copy link
Contributor

@jridgewell jridgewell commented Mar 9, 2021

This commit is purposefully forked several commits before (768808e) the current master (d110566). I'm going to rebase and merge to see what happens.

@jridgewell jridgewell force-pushed the build-size-base branch 3 times, most recently from 778cb10 to 0f8d04b Compare March 9, 2021 20:26
@jridgewell
Copy link
Contributor Author

jridgewell commented Mar 9, 2021

@jridgewell
Copy link
Contributor Author

I've now rebased on d876f32, creating 0f8d04b. Let's see how this one goes.

@jridgewell
Copy link
Contributor Author

jridgewell commented Mar 9, 2021

@rsimha
Copy link
Contributor

rsimha commented Mar 9, 2021

The rebase failed, but for a surprising reason! After the build, but before bundle-size, a new commit was merged onto master.

One way to avoid this is to report bundle sizes immediately after building in the same job (i.e., the old way). This probably means we'll need to merge the module and nomodule size reports on the server side instead of consolidating binaries during CI.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Mar 9, 2021

Hey @rsimha! These files were changed:

.circleci/config.yml
.circleci/fetch_merge_commit.sh

@jridgewell jridgewell force-pushed the build-size-base branch 5 times, most recently from e6259bd to fbebb49 Compare March 9, 2021 23:01
@jridgewell jridgewell changed the title Use the current master as the base point of bundle-size checks 🏗 Persist merge commit for reuse with later CI jobs Mar 9, 2021
@jridgewell
Copy link
Contributor Author

jridgewell commented Mar 9, 2021

The new persist logic worked! You can see we consistently used merge commit ba9a167, even though I purposefully merged #33062 after the build started (but before bundle-size started). This is proven by the changed commit log lines:

  *   ba9a1678e Justin Ridgewell - (HEAD -> pull/33165) Merge 0841e3d4929c06f6f0cbfa5938fe7594239e449d into 32a411de53fde79e349c6b636d8134acf070199d (4 minutes ago)
  |\  
  | * 0841e3d49 Justin Ridgewell - (origin/pull/33165) Fetch and store merge_commit at the beginning of the build (4 minutes ago)
- * | 32a411de5 Dima Voytenko - (origin/master, origin/HEAD, master) Multi-version: signal when extensions are known (#33162) (19 minutes ago)
+ * | 32a411de5 Dima Voytenko - Multi-version: signal when extensions are known (#33162) (27 minutes ago)

From that, we generated https://app.circleci.com/pipelines/github/ampproject/amphtml/4060/workflows/e50d7ea6-898d-455e-9c1e-ba6619708486/jobs/52305, and https://github.com/ampproject/amphtml/pull/33165/checks?check_run_id=2071240440.

@jridgewell
Copy link
Contributor Author

jridgewell commented Mar 9, 2021

I've now merged (using a merge commit) 6fc5bc6, creating 7f4eaee.

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Thanks for investigating this and prototyping a solution! You've managed to discover the elusive crux of the problem: a commit sneaking into master between build stages of a CI build.

However, I think the solution can be made more efficient at the minimum, and potentially be done without adding a new CI stage. Posting an initial comment inline, will follow up after doing some homework.

.circleci/config.yml Outdated Show resolved Hide resolved
@rsimha
Copy link
Contributor

rsimha commented Mar 10, 2021

And we have a successful app.circleci.com/pipelines/github/ampproject/amphtml/4064/workflows/1aa4cfb0-f652-4920-a78b-09e7dff2d126/jobs/52366/parallel-runs/0/steps/0-108 and #33165 (checks).

The "compare" link on this check #33165 (checks) shows code from other PRs, I believe it too needs to use the merge commit SHA instead of gitCommitHash().

Copy link
Contributor Author

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

The "compare" link on this check #33165 (checks) shows code from other PRs, I believe it too needs to use the merge commit SHA instead of gitCommitHash().

I agree partially. The comparison is definitely wrong, but I think the HEAD and BASE are should be reported as they are. We should instead change the code changes link to MERGE?

.circleci/config.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

I agree partially. The comparison is definitely wrong, but I think the HEAD and BASE are should be reported as they are. We should instead change the code changes link to MERGE?

Today, CI builds only report the base SHA to the bundle-size app (along with the actual sizes).

body: {
baseSha,
bundleSizes: await getBrotliBundleSizes(),
},

The app infers the head SHA and constructs the comparison link. We'll likely need to change this API to properly fix the comparison link.

.circleci/config.yml Outdated Show resolved Hide resolved
@jridgewell
Copy link
Contributor Author

Added a mergeSha field to the bundle-size endpoint POST. We just need to update the API to use it.

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

A few comments based on your latest changes.

.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/fetch_merge_commit.sh Show resolved Hide resolved
Comment on lines 197 to 204
/**
* Returns the merge commits SHA when running a CI Pull Request build.
* @return {string}
*/
function ciMergeSha() {
// CIRCLE_MERGE_SHA is populated by .circleci/fetch_merge_commit.sh.
return isCircleci ? env('CIRCLE_MERGE_SHA') : '';
}
Copy link
Contributor

@rsimha rsimha Mar 10, 2021

Choose a reason for hiding this comment

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

The thumb-rule for every function in this file is that it should be usable on GH actions if ever required, and that it is based on well-known env vars from each platform.

/**
* @fileoverview Provides various kinds of CI state.
*
* References:
* GitHub Actions: https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables#default-environment-variables
* CircleCI: https://circleci.com/docs/2.0/env-vars/#built-in-environment-variables
*/

For CIRCLE_MERGE_SHA, I wonder if we should just add a util function within bundle-size/index.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's not an equivalent GH env.

For CIRCLE_MERGE_SHA, I wonder if we should just add a util function within bundle-size/index.js.

I wanted to avoid tying the setup scripts to the actual JS code.

Copy link
Contributor

@rsimha rsimha Mar 10, 2021

Choose a reason for hiding this comment

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

SG. Let's just rename this function to circleciPrMergeSha to clearly indicate that it's not a generic CI function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request, the GITHUB_SHA is actually the merge commit! And we're mixing up the CCI SHA (which is the HEAD SHA) with the GH SHA (which is the merge SHA) in ciCommitSha above.

I'll rename this for now, and leave it to you to cleanup the env vars.

Copy link
Contributor

Choose a reason for hiding this comment

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

ciCommitSha is only used during push builds, and according to these docs, GITHUB_SHA is the correct variable.

I will admit that ci.js has gotten confusing due to all the individual use cases, and could do with a separate cleanup.

Other cleanup is coming up in #33215.

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for patiently dealing with all my review comments!

Comment on lines 197 to 204
/**
* Returns the merge commits SHA when running a CI Pull Request build.
* @return {string}
*/
function ciMergeSha() {
// CIRCLE_MERGE_SHA is populated by .circleci/fetch_merge_commit.sh.
return isCircleci ? env('CIRCLE_MERGE_SHA') : '';
}
Copy link
Contributor

@rsimha rsimha Mar 10, 2021

Choose a reason for hiding this comment

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

SG. Let's just rename this function to circleciPrMergeSha to clearly indicate that it's not a generic CI function.

.circleci/fetch_merge_commit.sh Show resolved Hide resolved
*/
function ciMergeSha() {
// CIRCLE_MERGE_SHA is populated by .circleci/fetch_merge_commit.sh.
return isCircleci ? env('CIRCLE_MERGE_SHA') : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this condition be isPullRequestBuild() && isCircleciBuild() to guarantee an empty value during push builds?

@jridgewell jridgewell merged commit 980f0ef into ampproject:master Mar 10, 2021
@jridgewell jridgewell deleted the build-size-base branch March 10, 2021 03:38
@jridgewell jridgewell restored the build-size-base branch March 10, 2021 04:42
@jridgewell
Copy link
Contributor Author

Lol, deleting my branch borked master. I restored it until #33176 can be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants