-
Notifications
You must be signed in to change notification settings - Fork 13
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
Comments
latest
only
@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 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. |
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. |
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. 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. |
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
Correct. It's a serious issue for many CIs. GH's way of doing it is really nice. |
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 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. |
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. |
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 onlatest
branch (or if you were more conservative, only run when a tag is created).Therefore can I suggest that an
if
conditional which makes thepublish
step only run when the branch islatest
? I've used this in my Vue project to skip publishing when the event type ispull_request
(which avoids hardcodinglatest
ormaster
).https://github.com/MichaelCurrin/badge-generator/blob/master/.github/workflows/main.yml#L51-L52
The text was updated successfully, but these errors were encountered: