[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:
Blocks
blocks FOLIO-3272 rollout GitHub workflow to the ui- UI... Closed
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.
Steps to Reproduce:

  1. Visit https://github.com/folio-org/ui-users/pull/1849/checks
  2. Click "Re-run jobs > Re-run all jobs"

Expected Results: Job runs successfully
Actual Results: "Run SonarCloud scan" step fails with this output:

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/
Implementing that simply will result in a compromised workflow. Need to still find a way to safely implement the same.

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

Generated at Thu Feb 08 23:27:01 UTC 2024 using Jira 1001.0.0-SNAPSHOT#100246-sha1:7a5c50119eb0633d306e14180817ddef5e80c75d.