-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
misc(build): give build-tracker a shared git history on PRs #11449
Conversation
# buildtracker runs `git merge-base HEAD origin/master` which needs more history than depth=1. https://github.com/paularmstrong/build-tracker/issues/106 | ||
- name: Deepen git fetch (for buildtracker) | ||
run: git fetch --deepen=100 | ||
# buildtracker needs history and a common merge commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we drop the entire script if we just skip this step if the current branch isn't master? so it only runs on commits merged to master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this script is mostly necessary for PRs.
though on master we need we need just 1 command... the git fetch --deepen
.
the bash script early exists (line 26) in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want it tracking PR? It doesn't ever fail, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want master for timeseries historical data
we want prs for budgets. once bt is running successfully i can add bundle size budgets, which is the last remaining item to complete the move from the old bundlesize
module to this.
buildtracker CLI won't have a non-zero exit code, even with failing budgets.. but i'll likely post to the status API to express the failure state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to check against a budget w/o uploading the data for the PR revision (and thus, no need to do this complex git stuff)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If build tracker UI can't filter to just master commits, I'm hesitant to mix the two data sources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If build tracker UI can't filter to just master commits, I'm hesitant to mix the two data sources.
totally. the UI filters to just master/default branch by default. so they're not mixed.
Is there a way to check against a budget w/o uploading the data for the PR revision (and thus, no need to do this complex git stuff)?
nope. while you can set 3 types of budgets (SIZE, DELTA, PERCENT_DELTA), the type we want is DELTA, which needs to know the baseline size, and only the server knows that. so in BT, the budgets are configured against the server instead of the client who's doing the build.
i asked the buildtracker maintainer about uploading non-master stuff and he said at twitter they upload all branches. BT may at some point allow switching the UI to look at a diff branch but its not implemented yet.
Can you add a link to the build tracker somewhere, maybe the readme? |
Is next action on reviewer here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
maybe the script should be inside .github/workflows
(or .github/workflows/scripts
)? It's not useful at all except for CI.
# Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
## | ||
|
||
set -euo pipefail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wanna add x
?
build-tracker relies on a common commit that's shared between HEAD and master.
Lighthouse runs CI on pull_request, not push, so the checkout is not the branch with shared history, but the result of a merge.
The checkout@v2 action uses a merge remote (eg.
remotes/pull/9605/merge
) that often has just a single commit. (single commit on these for our repo, not the case for other smaller repos... TBH i don't know how their logic works)This script creates a new branch that matches the current checkout, but does have a shared history.
ref #10472