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

Run CI publish step on latest branch only #13

Open
MichaelCurrin opened this issue Jan 11, 2021 · 6 comments
Open

Run CI publish step on latest branch only #13

MichaelCurrin opened this issue Jan 11, 2021 · 6 comments
Labels
enhancement New feature or request

Comments

@MichaelCurrin
Copy link
Contributor

MichaelCurrin commented Jan 11, 2021

I have PRs which are failing on this GH Actions publish step:

https://github.com/getmeli/meli-docs/blob/latest/.github/workflows/main.yml#L25

This is expected, as my fork does have secrets setup to publish to someone else's NPM or GH account.

Another problem is that even if the maintainer of the repo (who does have access) makes a PR from a branch to latest, the publish step will run on the branch (which may not be production ready). It would be safer to make the publish step on run when code is on latest branch (or if you were more conservative, only run when a tag is created).

Therefore can I suggest that an if conditional which makes the publish step only run when the branch is latest? I've used this in my Vue project to skip publishing when the event type is pull_request (which avoids hardcoding latest or master).

https://github.com/MichaelCurrin/badge-generator/blob/master/.github/workflows/main.yml#L51-L52

@MichaelCurrin MichaelCurrin changed the title Run CI publish step on latest only Run CI publish step on latest branch only Jan 11, 2021
@gempain
Copy link
Contributor

gempain commented Jan 11, 2021

@MichaelCurrin as I was started to look at your PRs, I noticed this and fixed it even before I got to this issue, great minds meet 😄 Sorry for the hickup, it's something I was aware of but had pushed back on the todo list as PRs weren't coming in this fast 😄

Using an if would be a good solution, but I went with a slightly different method. I created two workflows: one for PRs and another one for pushes. This has the advantage of letting any member of this project to deploy previews while external PRs will skip preview deployments.

You've actually pointed out something that has been bugging me for a while: Meli currently allows you to preview PRs for members of your project, but external PRs cannot be previewed because as you rightly pointed out, GH actions runs on your side and you don't have the secret to push to the repo. This is something I'd like to find a way around for Github - the problem doesn't happen on CircleCI and others, it's specific to how GH Actions works, which is quite interesting I think). I like how GH actions does CI because it prevents anyone to actually try to steal your secrets, which is something doable on most other CIs - and I don't understand why no one has actually started looking into this deeper, most of them delegate security management to the user.

The main problem is that letting external PRs deploy could allow anyone to start clogging your Meli's server with who-knows-what. Also, at the moment, site tokens allow you to override any branch, which means a PR could try to override your main branch. We'd need to introduce a new setting on tokens which would toggle on/off the ability to change the main branch, this way you could allow deploys on those external branches with a less destructive token.

Another solution would be to use webhooks. This way, when a PR comes in, Meli can download the content of the branch and load a preview. This would be the safest I think.

@MichaelCurrin
Copy link
Contributor Author

Ok good, glad you've got the two workflows idea then.

I had thought of two workflow files before for my own projects. But it means that any common steps like install, build and test get duplicated on two files and you have to remember to change both.

You can also use two jobs in one workflow file, and have the deploy job depend on on the install_build_test job, and you can set a condition on the deploy job itself (not just a step) run only on a push. But you have to do work to store output of one job and open it in the open job.

@MichaelCurrin
Copy link
Contributor Author

MichaelCurrin commented Jan 11, 2021

Regarding GH Actions, checkout out the Actions tab on Settings. Perhaps you want to try something more secure to stop anyone making a potentially dangerous PR when making a deploy preview.

This looks like you define the Action that is used - one within your org, a GitHub one, a marketplace action which needed to approval or a named action. This does NOT look like it cares whether the workflow runs on the original repo or on a fork.


Screen Shot 2021-01-11 at 2 38 15 pm


I believe GH Actions and Netlify will also strip out a token from the logs for security, but of course someone could try send it somewhere else (like adding their own malicious action to the workflow and passing and storing the token).

I am not so familiar with webhooks.

@gempain
Copy link
Contributor

gempain commented Jan 11, 2021

Ok good, glad you've got the two workflows idea then.

I had thought of two workflow files before for my own projects. But it means that any common steps like install, build and test get duplicated on two files and you have to remember to change both.

You can also use two jobs in one workflow file, and have the deploy job depend on on the install_build_test job, and you can set a condition on the deploy job itself (not just a step) run only on a push. But you have to do work to store output of one job and open it in the open job.

Right on 😄 the two workflows is a bit tedious as we're duplicating the basis, but here it's fine with me as it's just a few lines.

The multi-job approach is definitely cleaner, it was just quicker to have two workflows, but I just looked a bit more into this, and we could do an if statement on the second job with github.base_ref. We can then use artifacts for pass the build dir between both jobs.

I believe GH Actions and Netlify will also strip out a token from the logs for security, but of course someone could try send it somewhere else (like adding their own malicious action to the workflow and passing and storing the token).

Correct. It's a serious issue for many CIs. GH's way of doing it is really nice.

@MichaelCurrin
Copy link
Contributor Author

I have a simplified example of artifacts if you need https://michaelcurrin.github.io/dev-cheatsheets/cheatsheets/ci-cd/github-actions/jobs.html#job-sequence

Checking for if github.base_ref is shorter but less clear what it is doing than if gitub.event_name != 'pull_request.

Also, you might get an error that the key is not set, since it says the attribute is not always available. Or maybe it will return nil or empty string.

@gempain
Copy link
Contributor

gempain commented Jan 12, 2021

Thanks for the example ! I'll have a look tomorrow 😄

Regarding the value, I'll run a few tests on a demo project to see how this behaves.

@gempain gempain added the enhancement New feature or request label Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants