Skip to content

chore: add DAGGER_COMMIT option to install.sh#7544

Merged
jedevc merged 2 commits into
dagger:mainfrom
jedevc:install-arbitrary-commit
Jun 10, 2024
Merged

chore: add DAGGER_COMMIT option to install.sh#7544
jedevc merged 2 commits into
dagger:mainfrom
jedevc:install-arbitrary-commit

Conversation

@jedevc
Copy link
Copy Markdown
Contributor

@jedevc jedevc commented Jun 4, 2024

This allows installing an arbitrary commit from main, to make it easier to install versions from main for testing.

TODO:

  • Test install.ps1 changes on windows

This allows installing an arbitrary commit from main, to make it easier
to install versions from main for testing.

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc requested a review from matipan June 4, 2024 13:04
@jedevc
Copy link
Copy Markdown
Contributor Author

jedevc commented Jun 4, 2024

fyi @shykes

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc marked this pull request as ready for review June 5, 2024 09:25
@jedevc jedevc added this to the v0.11.7 milestone Jun 5, 2024
@jedevc jedevc requested a review from gerhard June 5, 2024 16:24
Comment thread install.ps1
$version = "v" + (latest_version)
}
$fileName="dagger_v" + $version + "_windows_amd64"
$fileName="dagger_" + $version + "_windows_amd64"
Copy link
Copy Markdown
Contributor

@pjmagee pjmagee Jun 6, 2024

Choose a reason for hiding this comment

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

I thought this PR was only for .sh but can you check my PR maybe for some improvements i.e:

  • The amd64 is fixed here but Windows runs ARM too WinRT
  • The $InteractiveInstall could be a [switch] for a cleaner looking .\script -Install (no bool)

#7569

you can close mine if some of my changes can improve this one enough?

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.

Thanks @pjmagee for #7569, they look pretty good to me!

I think both PRs do something different, this is just adding a new feature into both scripts to make it easier to consume builds from main. I think my idea is to try and merge this one first, and then we can rebase #7569 on top of that with changes?

Copy link
Copy Markdown
Contributor

@pjmagee pjmagee Jun 7, 2024

Choose a reason for hiding this comment

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

yeah, Lets do that. Though I do actually notice now that the .ps1 script was looking as close as possible to the .sh script. If thats a conscious decision by the dagger team, maybe my PR is not a good idea?

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.

The keeping it inline was mostly just me attempting to tidy up the windows script when I reimported it - there's no issue if you want to diverge IMO, if that's more familiar to windows users 🎉

@jedevc jedevc mentioned this pull request Jun 7, 2024
6 tasks
@gerhard
Copy link
Copy Markdown
Contributor

gerhard commented Jun 10, 2024

Taking a look at this now.

Copy link
Copy Markdown
Contributor

@gerhard gerhard left a comment

Choose a reason for hiding this comment

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

Code changes look right to me. I also checked that:

  • ./install.sh installs latest released version
  • DAGGER_VERSION=v0.11.5 ./install.sh installs specific version
  • DAGGER_COMMIT=2cbda9c7547809a35eb93ff7677e5bab4afd00c1 ./install.sh installs dev version
  • DAGGER_COMMIT=2cbda9c ./install.sh fails as expected (full git sha is required)

LGTM 👍 🚀

After this gets merged, DM me @jedevc so that I can do the CloudFront dance 🕺

@jedevc jedevc merged commit 0cd6002 into dagger:main Jun 10, 2024
@jedevc jedevc deleted the install-arbitrary-commit branch June 10, 2024 12:20
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