Enable fork-pull workflows

[rajsite]

🧹 Tech Debt

Currently nimble does not support fork-pull workflows which makes it difficult for contributions outside of the nimble team. We only support branch-pull and all contributors need to be added to the nimble GitHub team in order to create PRs.

Our beachball integration is currently normal enough that we should be able to switch to a fork-pull workflow. One thing to consider is that with fork-pull secrets are not included which is fine for publishing (github pages, npm, nuget) but may be annoying to lose our other analysis tools (lighthouse, chromatic).

We should switch to fork-pull, particularly to allow for outside contributors when open sourcing #290, but this does not need to be blocked on open sourcing and could be useful now, see #633.

[nate-ni]

Add instructions for this in CONTRIBUTING.md doc when this gets sorted to instruct contributors on what to do.

[rajsite]

Assigning myself to take another stab at addressing. Example simple PR that won't even run as is: #1752

[rajsite]

Current status:

Updated workflows to ignore steps that require secrets in: 2b0798a

• The goal of the change was to skip steps if a secret was not defined. You cannot check the value of secrets in the if conditional and the docs show creating env variables to hold the secrets but instead I went for making env variables that just hold if the secret was defined. Seemed unnecessary to make the secrets available in the env for every step.

So the workflow for a user that is not on the nimble team is like the following:

1. Create a fork of nimble
2. Optional, but recommended, in the fork go to the actions tab and enable actions so your branches run tests
3. Push you changes to main or a branch in your fork
   • If pushing to main: the workflow will run and try to publish the github pages site in your fork. It will fail unless you enable GitHub pages in your fork.
     • Maybe we should encourage enabling github pages in forks? Maybe we should make that step ignore failures? Maybe that step should not run in forks? The bulk of the benefit is seeing the Angular and Blazor built app and that can happen in PR as part of the chromatic build, see below. Decision: We won't encourage the github pages publish in forks, but we should make it so pushing to main outside ni skips publish.
   • If pushing to a branch: the workflow will run to success but you don't have any secrets so you don't get chromatic or lighthouse reports
     • Maybe we want to intentionally leak our chromatic project key? That wouldn't only be handy for running chromatic in the user fork. Not necessary for the the PR, see below. Decision: We don't want to leak the project key as we don't have great traceability of usage. We could have out to specific users who would have benefit (or maybe just add them as contributors). No action needed
   • In either scenario change files are computed against the main of the fork and not against upstream ni/nimble. Beachball did add code to detect the remote repository but it looks like the implementation checks against the remotes configured in git and as far as I can tell the github actions checkout step only configures the origin remote to be the fork and not an upstream remote to be ni/nimble.
     • I think we need to add a step in the workflow to configure nimble as an upstream, i.e. git remote add upstream <ni/nimble repo url> (that should be safe to always) or we disable beachball change detection from builds in the fork (make them deal with it only in PR, and even then nimble owners can push the change directly to the branch generally)
       • disabled beachball change detection for outside ni
4. Create a pull request to nimble with the changes
   • When the PR is opened, by default the workflow won't run for a user that is not part of the nimble team. Someone from nimble needs to approve the workflow run for each commit:
     
   • Secrets are not exposed to workflows triggered from pull_request by users not in the nimble team so we lose chromatic and lighthouse status checks. As the chromatic status check is required to merge any PR that means the PR cannot be merged without admin override.
     
     • The approach to enable chromatic and lighthouse checks from PRs without leaking credentials is to do the pull_request + save artifacts + workflow_run workflow
       • 3/6/24: started looking into this
         • Maybe helpful, use the download-artifact action: support for "workflow_run" event triggers actions/download-artifact#60 (comment)
         • Issue: performance isn't a single artifact anymore, it's multiple checks that would upload to lighthouse. But maybe they would actually work and give a status check with just the normal GITHUB_TOKEN based to workflow_run? didn't try
         • Issue: chromatic action seems to require a git checkout to work based on docs which we would not have, we would only have the storybook artifact. Maybe they have working support for workflow_run? Didn't see if it works in forks. And if they do then why is it not discussed in the docs?
         • Alternative: Maybe we can just include the token in workflow like the docs say. Do they do any checks on the token to prevent abuse?
         • Alternative: Maybe we can do a workflow_dispatch and pass the chromatic tokens manually?

[jattasNI]

This issue is currently deprioritized so for now if an external contributor creates a PR then a maintainer will need to copy their changes into a new branch and create a new PR in order for it to be submitted to the repo.

We should capture this in CONTRIBUTING.md and consider closing this issue.