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

Fix size-check to display more context #13955

Merged
merged 1 commit into from
Apr 21, 2022

Conversation

cevich
Copy link
Member

@cevich cevich commented Apr 21, 2022

When going through the rebase+build loop, the repository state won't
match the exact branch or PR history. This results in the Building: XYZSHA indications being entirely useless. Fix this by at least
including the title line of the commit being built. This will allow a
human to make sense of any size-check failure WRT their view of history.

Signed-off-by: Chris Evich [email protected]

When going through the rebase+build loop, the repository state won't
match the exact branch or PR history.  This results in the `Building:
XYZSHA` indications being entirely useless.  Fix this by at least
including the title line of the commit being built.  This will allow a
human to make sense of any size-check failure WRT their view of history.

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich requested a review from edsantiago April 21, 2022 18:09
@cevich
Copy link
Member Author

cevich commented Apr 21, 2022

Example log from PR #13376, Building: 2ca973df05ababc044e623cb92813c8e9a22d71f where check fails, doesn't correspond to any commit ID on the base branch or the PR. The base-branch HEAD just happened to be a commit which grew the binary size beyond the threshold (and presumably had the magic label set).

@cevich
Copy link
Member Author

cevich commented Apr 21, 2022

Note: The effect of this PR can't be observed until the changes land in main 😢

@edsantiago
Copy link
Member

That seems odd: I distinctly remember going through Cirrus logs confirming that hashes matched 1:1 to those on my laptop - and they did, otherwise, as you say, the hashes would be useless. I think I would've been happier with an explicit --format='^h %aI %s' but this is close enough. Thanks for taking care of it.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2022
@@ -92,9 +92,10 @@ if [[ ! -d $context_dir ]]; then
fi

# This is the original (and primary) purpose of this check: if 'make' fails,
# there is no point in continuing
# there is no point in continuing. Show at least the commit title since
# the ID may not match anything human recognisable.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# the ID may not match anything human recognisable.
# the ID may not match anything human recognizable.

Copy link
Member Author

Choose a reason for hiding this comment

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

damn, codespell must not be turned on.

@TomSweeneyRedHat
Copy link
Member

LGTM
nit fix in comment can be fixed (or not) at your leisure

@TomSweeneyRedHat
Copy link
Member

/lgtm

@cevich
Copy link
Member Author

cevich commented Apr 21, 2022

and they did

I believe you. In case it helps, I did go poking around on the VM while the test was running and it appeared like maybe the 'merge' commits were being skipped over. I wasn't looking very carefully though, but it could easily be something like that. Maybe something in your gitconfig hid the problem?

@edsantiago
Copy link
Member

Dunno. But your approach is better regardless, thanks!

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 21, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, edsantiago

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 21, 2022
@openshift-merge-robot openshift-merge-robot merged commit bfd617e into containers:main Apr 21, 2022
@cevich cevich deleted the fix_size_check branch April 18, 2023 14:50
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 28, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants