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: restore install.ps1 #7050

Merged
merged 3 commits into from
Apr 16, 2024
Merged

chore: restore install.ps1 #7050

merged 3 commits into from
Apr 16, 2024

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Apr 8, 2024

Fix #6901.

This script was removed in 5a9c772, and we still have windows install instructions, so we should probably restore this!

I've also updated the script with a few additions, like making the function names map to the same as the unix script.

@jedevc jedevc requested review from gerhard and matipan April 8, 2024 14:40
.goreleaser.yml Outdated
Comment on lines 57 to 58
- name: publish-install-sh
cmd: sh -c "aws s3 cp install.sh s3://{{ .Env.AWS_BUCKET }}/dagger/install.sh"
Copy link
Member Author

Choose a reason for hiding this comment

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

From what I can tell, there's currently no automation for this right now? This is what prevented install.ps1 from being updated in the docs in the first place.

If so, I think this is the right way to do it, we should have goreleaser release the install script.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense to me if it works and has thumbs up from @matipan or @gerhard 👍

Copy link
Member

@gerhard gerhard Apr 15, 2024

Choose a reason for hiding this comment

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

We do this manually because we need to invalidate CloudFront too. FTR:

If we want to do this properly, we should add it as a function - follow-up to:

If we want to do this quick & dirty, we need to also invalidate CloudFront. Relevant past comments:

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good! Hadn't realized that, I've updated the RELEASING guide to include info about doing this.

install.ps1 Outdated
Comment on lines 10 to 16
@"
---------------------------------------------------------------------------------
Author: Alessandro Festa
Co Author: Brittan DeYoung
Dagger Installation Utility for Windows users
---------------------------------------------------------------------------------
"@
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about keeping this, none of our other install scripts or tools have author names like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a commit to convert this author block to a comment instead of it being output on every execution.

Copy link
Contributor

@jpadams jpadams left a comment

Choose a reason for hiding this comment

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

If others can confirm infra side, let's ship it!

.goreleaser.yml Outdated
Comment on lines 57 to 58
- name: publish-install-sh
cmd: sh -c "aws s3 cp install.sh s3://{{ .Env.AWS_BUCKET }}/dagger/install.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense to me if it works and has thumbs up from @matipan or @gerhard 👍

# Author: Alessandro Festa
# Co Author: Brittan DeYoung
# Dagger Installation Utility for Windows users
# ---------------------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

This script works fine for me. Now just need the dagger.exe that it downloads to give some output. That's the other PR :)

@jedevc jedevc added this to the v0.11.1 milestone Apr 15, 2024
Copy link
Member

Choose a reason for hiding this comment

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

This looks good to me. I haven't tested, but the shape is correct. FTR:

image image

jedevc and others added 3 commits April 15, 2024 17:37
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Jeremy Adams <jeremy@dagger.io>
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc requested a review from gerhard April 15, 2024 16:42
@jedevc jedevc merged commit aeab44a into dagger:main Apr 16, 2024
40 of 43 checks passed
@jedevc
Copy link
Member Author

jedevc commented Apr 16, 2024

Merging this since the script looks good - even if this ends up wrong, we can also update this on S3 async to our release process.

@jedevc jedevc deleted the restore-install-ps1 branch April 16, 2024 10:31
vikram-dagger pushed a commit to vikram-dagger/dagger that referenced this pull request May 3, 2024
* chore: restore install.ps1

Signed-off-by: Justin Chadwell <me@jedevc.com>

* chore: Make author block comment instead of stdout

Signed-off-by: Jeremy Adams <jeremy@dagger.io>

* chore: update RELEASING to mention install scripts

Signed-off-by: Justin Chadwell <me@jedevc.com>

---------

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Jeremy Adams <jeremy@dagger.io>
Co-authored-by: Jeremy Adams <jeremy@dagger.io>
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.

Windows installation instructions do not work
3 participants