Skip to content

Fix arch is undefined, update tests, and allow manual invocation of action#8

Merged
r-LaForge merged 13 commits intomainfrom
ryan/enable-running-gha-manually
Sep 20, 2022
Merged

Fix arch is undefined, update tests, and allow manual invocation of action#8
r-LaForge merged 13 commits intomainfrom
ryan/enable-running-gha-manually

Conversation

@r-LaForge
Copy link
Copy Markdown
Contributor

@r-LaForge r-LaForge commented Sep 20, 2022

Fix arch is undefined, update tests, and allow manual invocation of action.

Earthfile Outdated
Comment on lines +55 to +56
RUN export PATH="$(cat earthly-path):$PATH" && earthly --version
RUN export PATH="$(cat earthly-path):$PATH" && earthly --version | grep '^earthly version v.* [a-zA-Z0-9]* linux/amd64; Alpine Linux'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Running as a separate command simply makes it easier to see the version to make the necessary changes. This test relies on the specific format of our version command.

Comment on lines +3 to +13
exports[`get-version range versions should match ^0 versions 1`] = `"v0.6.23"`;

exports[`get-version range versions should match 0.*.* versions 1`] = `"v0.6.14"`;
exports[`get-version range versions should match 0.*.* versions 1`] = `"v0.6.23"`;

exports[`get-version range versions should match 0.4.* versions 1`] = `"v0.4.6"`;

exports[`get-version range versions should match 0.6.1 versions 1`] = `"v0.6.1"`;

exports[`get-version range versions should match 0.6.1 versions 2`] = `"v0.6.1"`;

exports[`get-version range versions should match latest versions 1`] = `"v0.6.14"`;
exports[`get-version range versions should match latest versions 1`] = `"v0.6.23"`;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

snapshots depend on the current version of Earthly. They need to be updated every time we do work after releasing a new version.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ouch!

Maybe we should parse the string into [0, 6, 23] then we can just programatically check that it's greater than [0,6,1].

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I decided to use semver for this. Made the tests very straightforward.

Comment on lines +40 to +41
const osArch = os.arch();
const releaseArch = nodeArchToReleaseArch[os.arch()] || osArch;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When the osArch wasn't in the map we got undefined and cached linux-undefined.

@r-LaForge r-LaForge changed the title [ci]: Add workflow_dispatch event to allow running manually Fix arch is undefined, update tests, and allow manual invocation of action Sep 20, 2022
@r-LaForge r-LaForge requested a review from alexcb September 20, 2022 20:57
Earthfile Outdated
RUN export PATH="$(cat earthly-path):$PATH" && earthly --version | grep '^earthly version v.*linux/amd64; Linux'
# [a-zA-Z0-9]* attempt to match a commit hash
RUN export PATH="$(cat earthly-path):$PATH" && earthly --version
RUN export PATH="$(cat earthly-path):$PATH" && earthly --version | grep '^earthly version v.* [a-zA-Z0-9]* linux/amd64; Alpine Linux'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it looks like this regex now requires two spaces for cases like v0.1.2<space><space>linux/amd64

should it be something closer to:

Suggested change
RUN export PATH="$(cat earthly-path):$PATH" && earthly --version | grep '^earthly version v.* [a-zA-Z0-9]* linux/amd64; Alpine Linux'
RUN export PATH="$(cat earthly-path):$PATH" && earthly --version | grep '^earthly version v.*( [a-zA-Z0-9]+)? linux/amd64; Alpine Linux'

However note that there's already a .* should should match everything between v and linux/amd64, so I'm not sure about this regex change.

What about:

RUN export PATH="$(cat earthly-path):$PATH" && earthly --version | tee version.output
RUN grep '^earthly version v.*linux/amd64; Linux' version.output

instead?

Copy link
Copy Markdown
Contributor Author

@r-LaForge r-LaForge Sep 20, 2022

Choose a reason for hiding this comment

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

This is very close.
I've adjusted the regex you've provided just a bit to allow for the specific flavour of Linux as well (Alpine in this example).

RUN grep '^earthly version v.*linux/amd64; Alpine Linux' version.output

Copy link
Copy Markdown
Contributor

@alexcb alexcb left a comment

Choose a reason for hiding this comment

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

looking good!

I left a couple of small suggestions to make this more manageable in the future.

@r-LaForge r-LaForge requested a review from alexcb September 20, 2022 22:11
RUN cat output | grep '^::add-path::' | sed 's/::add-path:://g' > earthly-path
RUN test "$(cat earthly-path)" = "/root/.earthly/bin"
RUN export PATH="$(cat earthly-path):$PATH" && earthly --version | grep '^earthly version v.*linux/amd64; Linux'
# [a-zA-Z0-9]* attempt to match a commit hash
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this comment still relevant?

@r-LaForge r-LaForge merged commit c2d4f80 into main Sep 20, 2022
@r-LaForge r-LaForge deleted the ryan/enable-running-gha-manually branch September 20, 2022 23:00
@renovate renovate bot mentioned this pull request Sep 30, 2025
1 task
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.

2 participants