[FOLIO-3290] PRs from forks cannot be merged due to missing SONAR_TOKEN value Created: 16/Sep/21 Updated: 26/Oct/21 Resolved: 26/Oct/21 |
|
| Status: | Closed |
| Project: | FOLIO |
| Components: | None |
| Affects versions: | None |
| Fix versions: | None |
| Type: | Bug | Priority: | TBD |
| Reporter: | Zak Burke | Assignee: | Ankita Sen |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original estimate: | Not Specified | ||
| Issue links: |
|
||||||||
| Sprint: | DevOps Sprint 123, DevOps Sprint 124, DevOps Sprint 125, DevOps Sprint 126 | ||||||||
| Development Team: | FOLIO DevOps | ||||||||
| Description |
|
Overview: PRs from forks of repos built with GitHub Actions cannot be merged due to missing SONAR_TOKEN value. PRs from branches directly on the origin repository are not affected.
Expected Results: Job runs successfully Set the SONAR_TOKEN env variable. despite the .github/workflows/buildnpm.yml file containing a setting for this value. Interested parties: Christian Cruz, Yasmine Macedo R, Isela García Bravo |
| Comments |
| Comment by Ankita Sen [ 16/Sep/21 ] |
|
Zak Burke - Is the problem happening only when the repository is forked? The SONAR_TOKEN variable is set as a Oragnisation level secret so that there is no need to add this to all individual repositories. That might be causing this bug. This needs to set, I think along with all other organisational secrets when the repository is forked, by the organisation who is forking the repository. There might be a better way to do this as well. I can have a look into it. |
| Comment by Holly Mistlebauer [ 16/Sep/21 ] |
|
This needs to be fixed ASAP as our code freeze is tomorrow...thanks... |
| Comment by Zak Burke [ 16/Sep/21 ] |
|
Ankita Sen, Holly Mistlebauer: the easy workaround is for me to just pull down their forked branches and push 'em back to origin to create a replacement PR from a branch within folio-org. This works like a charm (git history is correctly preserved) and is not onerous. Long term, it's worth investigating solutions that would allow folks on forks to run their PRs through the whole CI process so they don't have to rely on somebody within folio-org to test their work. |
| Comment by Ankita Sen [ 21/Sep/21 ] |
|
Update : Following the implementation given in https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/ This was the best fitting solution I could get and coming from GitHub docs, seems like most viable. |
| Comment by Ankita Sen [ 23/Sep/21 ] |
|
Update: The pull_request_trigger has some security issues about exposing Secrets and providing read/write of the repository access to the PR author mentioned here https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ |
| Comment by Ankita Sen [ 04/Oct/21 ] |
|
Zak Burke : I have slightly changed the implementation. Now there is an identical workflow called build-npm-fork.yml which will run every time a forked repository creates a pull_request but only after the label 'safe fork' has been added to it as discussed before. This makes sure that our workflows are not compromised from a security pov. I have added the label 'safe fork' in the ui-users repository. Now if one of you could test it, then this is done. |
| Comment by Jakub Skoczen [ 04/Oct/21 ] |
|
Ankita Sen we discussed documenting the above on the FOLIO dev website. |
| Comment by Zak Burke [ 05/Oct/21 ] |
|
Ankita Sen, I did not see the safe fork label in ui-users so I created it, opened a PR from a fork, and added that label. SonarCloud still failed. I see
...
DEFAULT_BRANCH: master
GITHUB_TOKEN: ***
SONAR_TOKEN:
...
which makes it look like SONAR_TOKEN is still not being populated correctly. |
| Comment by Ankita Sen [ 05/Oct/21 ] |
|
Zak Burke - The updated workflow is missing in the forked repo because I didn't merge it with master. Will do so now and hopefully doing so will give us proper results. Once I have merged with the fork workflow, could you update the forked repo and try again?
|
| Comment by Jakub Skoczen [ 14/Oct/21 ] |
|
Ankita Sen Just to confirm that we are going to abandon the approach with labels and simply document the limitations for PRs from forks. |
| Comment by Ankita Sen [ 25/Oct/21 ] |
|
Have removed the *build-npm-fork.ym*l workflow and instead of a separate workflow, have added the workaround in the documentation I have added David Crossley as reviewer in this PR. |
| Comment by Ankita Sen [ 26/Oct/21 ] |
|
The limitations and the workaround to fix this problem has been added in to dev.folio.org documentation |