canonical / ds25

Canonical's design system
GNU Lesser General Public License v3.0
2 stars 1 forks source link

feat: Create CD Pipeline for NPM publishing #24

Closed jmuzina closed 4 days ago

jmuzina commented 1 week ago

Done

1/2 of WD-16919, along with #27

QA

PR readiness check

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

bartaz commented 5 days ago

How are we dealing with pushing updated versions to the repository?

As far as I understand lerna version does update and commit the updated versions (to package.json), but if we don't push them to the repo, they won't end up being "saved". That's the issue we have currently with automatic releases in react-components, that the versions don't get updated in the codebase at all, because main is a protected branch and it's not trivial to allow CI to push to protected branches.

Not sure how we want to deal with this issue here.

jmuzina commented 5 days ago

One oversight : Publish should be hooked on new tags starting with the prefix. Version should be a precondition for it (creates the right tag).

@advl One problem with this: tag pushes with GITHUB_TOKEN will not trigger a workflow that is started on tag pushes, to prevent recursive workflow runs per docs.

We could use a PAT instead, or trigger on creation of releases linked to tags (but the latter option would require triggering two workflows manually, which we should be sure we want)

jmuzina commented 5 days ago

main is a protected branch and it's not trivial to allow CI to push to protected branches.

@bartaz Good point. main is a protected branch on this repo, but not on my fork (where I tested this). This is currently working by pushing to main on the CI side.

Maybe a good way to handle this is to push the tags to a unprotected branch "release", then releasing can be done as a separate PR from "release" to "main", that way we can respect the branch protection rules.

jmuzina commented 5 days ago

Met with @advl to discuss publish triggering and branch protection questions.

Answers to both questions are below:

  1. Publish triggering (how will we trigger the publish workflow after the versioning - do we make a separate workflow running on tag creation? Is the publishing triggered by the same workflow as the versioning? How to resolve the GITHUB_TOKEN recursion protections?)

We will publish at the end of the versioning workflow, with a second job. First job builds, tests, versions, and commits, and the second job builds & publishes.

  1. Branch protection (how will we push version changes / bumps to the protected branch? Will a contributor run lerna version manually as a pre-PR step, or will we run lerna version in the CD itself?)

@jmuzina to look into Github Deploy keys as a mechanism for allowing the CI to bypass the branch protection policy. This way we can continue to trigger the versioning with a workflow dispatch.

jmuzina commented 5 days ago

Updated PR to address branch protection issue by using deploy keys.

The CI runner uses an SSH deploy key to authenticate to GitHub and push changes through the branch protection policy, per suggestion by @advl .

Taking a step back: I wonder if this is the best way of doing this. This would hypothetically allow anyone with write access to the repo to push to main by manipulating workflow files, disregarding the branch protection policies.

Alternate approaches:

  1. The current workflow is modified to create a version bump PR instead of immediately pushing to main. Approving and merging that PR triggers a workflow (based on push to main's lerna.json) that publishes the packages. This has the benefit of not requiring PR creators to decide on versions themselves.
  2. PR creator manually runs bun run version:<semver_level> before creating the PR, including the version bumps in each PR. This approach has the benefit of making main always releasable and keeping versioning decisions (semver level choice) very close to the code changes that cause a version bump. Then, a workflow can watch for release actions and run lerna publish. The downside is that a contributor must know to run the versioning script before opening a PR.
bartaz commented 5 days ago

I struggle to understand the workflow(s) just from the code (that is scattered across multiple files).

Do we have the proposal documented somewhere? Is this fully automatic (whenever any PR is merged) or is the trigger manual (if so, who/when triggers it)?

My understanding from talking with Adrian before was that we are aiming for full automation. That's why I suggested we try to make the CI job that changes the version to push it to main branch (with proper privileges needed for protected branches).

I'd really like to avoid setting the version manually in PRs. Current Vanilla workflow is confusing and prone to errors because of this requirement.

On the other end, current react-components workflow is fully automatic (each PR potentially triggers a release - based on semantic-release package deciding if the release should happen), but it doesn't update version in package.json, because we didn't have time to look into how to set it up with branch protection.

@jmuzina You mention some security issues with workflows being able to push to protected branch. What would be a potential scenario when this could be exploited?

jmuzina commented 5 days ago

Do we have the proposal documented somewhere? Is this fully automatic (whenever any PR is merged) or is the trigger manual (if so, who/when triggers it)?

@bartaz Workflow is documented here - it is triggered by repository maintainer using workflow dispatch

@jmuzina You mention some security issues with workflows being able to push to protected branch. What would be a potential scenario when this could be exploited?

This is really the core of my concern. With the approach currently in the PR, there is a SSH deploy key secret that actions run on the upstream repo can access. The branch protection policy is set to allow pushes made with this key to bypass branch protection policies, allowing package.json to be updated.

A malicious maintainer could make a branch on the upstream, modify the tagging workflow in such a way that arbitrary code is pushed, and run the workflow. They could also exfiltrate the key and use it to push to main on their own client.

It's an edge concern, I know, just want to be sure we're conscious of it.