marcelropos / HM-DiscordBot

A Discordbot by and for first semester students of HM.
8 stars 5 forks source link

changed the workflow #190

Closed maxwai closed 11 months ago

maxwai commented 11 months ago

changed so that the workflow on pull requests is not done twice when pushing to a branch that already has a pull request open.

Also changed that containers are only build on pushes to main as pull requests from forks don't have access to the secrets needed to push a container

schitcrafter commented 11 months ago

If I have a pull request from a fork to this repo, and push more commits to the base branch, will this run the action again? Because I don't think it will, push is only activated for branches on this repo while pull_request opened/reopened is only activated when the pr is, well, opened or reopened.

maxwai commented 11 months ago

Because I don't think it will, push is only activated for branches on this repo while pull_request opened/reopened is only activated when the pr is, well, opened or reopened.

You're right, I need to change that.

schitcrafter commented 11 months ago

One possibility might be push only on main, and then all pull requests. That would include everything on main + all branches, extern or not, that have an active pr. only possibility of executing twice is from a pr with base main, which shouldn't happen. (copied from poem) We could also narrow down to only pull_requests targetting main, but I wouldn't.

maxwai commented 11 months ago

@schitcrafter this should now the fixed. We now ignore synchronize action on pull request where the branch is in the main repo

maxwai commented 11 months ago

nvm doesn't work yet

maxwai commented 11 months ago

Ok, the conditional run of check_code works, but I noticed that pushing docker container on from a pull request of a fork doesn't work because the secrets are not shared

maxwai commented 11 months ago

Ok, changed the docker build action to only run on push to main since the secrets can't / shouldn't be shared on pull requests of forks since this could lead to secrets being leaked.

schitcrafter commented 11 months ago

Ok, changed the docker build action to only run on push to main since the secrets can't / shouldn't be shared on pull requests of forks since this could lead to secrets being leaked.

Pretty sure this isn't a concern: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ Look under the segment starting with "Due to the dangers", it clearly states that with "pull_request", only pr's from branches on this repo will have access to secrets. Although, I still don't see why we would want to build a docker container for every pull request - maybe to test the changes in a production environment?

I'll not merge this right now because of the discussion still going on

maxwai commented 11 months ago

Although, I still don't see why we would want to build a docker container for every pull request - maybe to test the changes in a production environment?

The idea was to check before merging that the container can be build and works. But I now recon a simple build without push should be fine on pull request and pushing only on push on main.

schitcrafter commented 11 months ago

Well, I'd say that the actions would certainly work like this - don't think the solution is very good, but if it's stupid and works it's not stupid. I'll approve it