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

chore: Add check for missing Alpine CHECKSUM #1317

Merged
merged 2 commits into from
Oct 16, 2021

Conversation

nschonni
Copy link
Member

The basic check works, but it would be better if the error can be set on the line in the affected file https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions#setting-an-error-message

Opening as a draft so someone better at bash can suggest something to pipe the find result in a better manner, or just to loop over the grep results

@@ -12,7 +12,7 @@ RUN addgroup -g 1000 node \
&& case "${alpineArch##*-}" in \
x86_64) \
ARCH='x64' \
CHECKSUM="2afa21473a5eb8f407c52fe3bf08180630f3ac9541d52b98e99dbf4fe79f82d2" \
CHECKSUM="" \
Copy link
Member Author

Choose a reason for hiding this comment

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

Here for testing, undo before landing

@@ -12,7 +12,7 @@ RUN addgroup -g 1000 node \
&& case "${alpineArch##*-}" in \
x86_64) \
ARCH='x64' \
CHECKSUM="2afa21473a5eb8f407c52fe3bf08180630f3ac9541d52b98e99dbf4fe79f82d2" \
CHECKSUM="" \
Copy link
Member Author

Choose a reason for hiding this comment

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

Here for testing, undo before landing

if [ "$(find -path *alpine*/Dockerfile -exec grep -l CHECKSUM=\"\" {} \; | wc -l)" == 0 ]; then
exit 0
else
# echo "::error file=FILENAME,line=15,col=22::Missing pre-built checksum"
Copy link
Member Author

Choose a reason for hiding this comment

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

This echo should set comments directly on the affected lines if the right filenames are printed. I've hardcoded the 15 and 22 line/col based off the templates

@SimenB
Copy link
Member

SimenB commented Aug 21, 2020

Thoughts on just throwing (exiting with an error?) in the update script instead?

@nschonni nschonni mentioned this pull request Sep 24, 2020
Base automatically changed from master to main March 15, 2021 16:23
@nschonni nschonni marked this pull request as ready for review October 16, 2021 19:14
@nschonni nschonni force-pushed the missing-checksum branch 3 times, most recently from a2aa52a to 1f002d6 Compare October 16, 2021 19:36
Co-authored-by: Mason Malone <mason.malone@gmail.com>
@nschonni
Copy link
Member Author

Rebasing off the test commit, but it looks good
image

@nschonni nschonni requested a review from a team October 16, 2021 19:54
@nschonni nschonni merged commit 6149e33 into nodejs:main Oct 16, 2021
@nschonni nschonni deleted the missing-checksum branch October 16, 2021 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants