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

Use craft master #22

Closed
wants to merge 1 commit into from
Closed

Use craft master #22

wants to merge 1 commit into from

Conversation

chadwhitacre
Copy link
Member

@chadwhitacre chadwhitacre commented Feb 10, 2022

getsentry/publish#666

What are the security implications here? Anyone with write access to the craft repo can already inject code into the publish repo (so actually we should limit who can push to master on craft to the same set of people that can manage publish), which has all of the sensitive GH secrets. This action is used in less-sensitive repos. I think we're okay.

sudo curl -sL -o /usr/local/bin/craft "$craft_url"
sudo chmod +x /usr/local/bin/craft
- name: Craft Prepare
- uses: docker://getsentry/craft:latest
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 took this from the publish yaml.

Copy link
Contributor

Choose a reason for hiding this comment

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

The default entrypoint for crafts docker image is craft binary. Without changing it, nothing will most likely(?) work, that's why its modified in the mentioned yaml - https://github.com/getsentry/publish/blob/737df8f68fbd4d237ff365de964a321ba0ef3bf8/.github/workflows/publish.yml#L66-L67

Copy link
Member

@BYK BYK Feb 11, 2022

Choose a reason for hiding this comment

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

This will significantly slow down preparations as the Docker image is huge. Also what @kamilogorek said, you cannot just use run: etc with a custom docker image.

Copy link
Member

Choose a reason for hiding this comment

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

It is a bit tricky to figure out but my recommendation would be to download the latest build artifact from Craft master instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the tips! :)

sudo curl -sL -o /usr/local/bin/craft "$craft_url"
sudo chmod +x /usr/local/bin/craft
- name: Craft Prepare
- uses: docker://getsentry/craft:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

The default entrypoint for crafts docker image is craft binary. Without changing it, nothing will most likely(?) work, that's why its modified in the mentioned yaml - https://github.com/getsentry/publish/blob/737df8f68fbd4d237ff365de964a321ba0ef3bf8/.github/workflows/publish.yml#L66-L67

@chadwhitacre chadwhitacre deleted the cwlw/craft-master branch November 22, 2022 19:49
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