Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Package publishing workflow and Angular packaging update [Retroactive PR] #207

Closed

Conversation

rajsite
Copy link
Member

@rajsite rajsite commented Nov 23, 2021

Pull Request

🤨 Rationale

This PR covers the changes associated with TASK 1744736 and covers the commits pushed directly to main to address the task.

Note: This PR will not be merged, it is being used to gather feedback for a follow-up PR.

The goal of this change was to be able to customize the publish behavior for Beachball during the beachball publish action (which does versioning and publishing) and this was accomplished by completely decoupling package versioning from package publishing.

Before these changes the workflow was as follows:

  • main workflow runs build and test
  • main workflow runs publish
    • beachball assumes each package is a normal npm package (build contents are stored in package directory and npm publish can be called in a directory)
    • Angular does not create normal npm packages, instead it forms a dist folder outside of the package folder with a different custom generated package.json and folder structure
    • We had a workaround that ran during the npm pack phase to mutate the package on disk to copy in the contents of the external dist folder temporarily so beachball can publish it as if it was a normal npm package
      • This is a bit messy and error prone and an oversight in this process when switching to ViewEngine based libraries was one of the issues resulting in the sl-header build stack overflows
    • beachball publish -> creates a temporary branch, updates the package.jsons for new versions, calls npm publish in each package (which causes the Angular pack dance above), pushes the branch remote as a new commit with tags, reverts the temporary branch locally in the CI
  • main workflow creates a lockfile by pulling in the remote branch, running npm install (which relies on the published packages existing in npmjs.com), and commits the new lockfile in a separate commit.

So at a high-level changes to the above workflow are the following:

The new workflow is as follows:

  • main workflow runs build and test
  • main workflow runs publish
    • beachball publish updates the package jsons, updates the lockfile with the new workaround, and creates tags and pushes them, but does not push to npm
  • tag workflows run once for each tag (currently up to three; nimble-angular, nimble-components, nimble-tokens)
  • tag workflows run build (but do not re-run tests, assumes tags came from main and are tested)
  • tag workflows run specific publish action
    • which is npm publish inside the package for normal packages and custom behavior for npm (running npm publish in the dist folder)

Pros:

  • Can publish out of the Angular dist folder which is Angular's expected workflow instead of the workaround needed to pretend to be a normal npm package.
  • Can do more advanced publishing for a specific tag, ie next step is add the ViewEngine build to create a separate parallel package built with View Engine
  • The lockfile update workaround does not pull from the registry. This was always a concern because it can pull in random updates and it does it at a dangerous stage after all testing has run. The lockfile update can pull in new packages that evade all testing and is immediately published.

Cons:

  • Each tag is a separate git object and triggers a separate build. They all run in parallel so it's not too bad time wise but it is wasteful in resources.
  • The publish is being done from a separate build that did not undergo testing. The goal of lockfiles etc is reproducible builds so if that is working correctly this is not a practical concern. Just feels a bit better knowing the exact file that was published is one that was tested. Probably wouldn't increase complexity to reproduce that though.
  • The package-lock.json update is done from a brute force text search and replace rather than using tools that can safely manipulate a package-lock.json

👩‍💻 Implementation

See above.

🧪 Testing

Did it live, in production :)

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed. I don't think it impacts developers, just infrastructure changes.

# Pull Request

## 🤨 Rationale

- Prevent beachball from publishing to npm (but preserve package bumps and git tags) @ni/nimble-angular
- Remove workarounds to enable beachball publishing of @ni/nimble-angular
- Add separate pipeline to publish @ni/nimble-angular

## 👩‍💻 Implementation

See above.

## 🧪 Testing

Doing it live.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike through~~ around items that don't (along with an explanation). -->

- [x] I have updated the project documentation to reflect my changes or determined no changes are needed. Skipped for now.
# Pull Request

## 🤨 Rationale

Continues on #203

- Disables beachball npm package publishing in CI (keeps package versioning and tagging)
- Moves all npm package publishing to separate workflow

## 👩‍💻 Implementation

See above.

## 🧪 Testing

Doing it live.

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or determined no changes are needed. Will follow-up after.
Beachball uses yargs under the hood which has the --no- convention:
https://github.com/yargs/yargs/blob/main/docs/api.md
…ld) (#205)

# Pull Request

## 🤨 Rationale

Minor edit made to nimble-tokens CONTRIBUTING.md to test publishing infrastructure changes.

## 👩‍💻 Implementation

Updated nimble-tokens CONTRIBUTING.md

## 🧪 Testing

Doing it live.

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or determined no changes are needed. Coming later.
# Pull Request

## 🤨 Rationale

A minor change to tokens to trigger a multipackage publish and test of the publishing workflows.

## 👩‍💻 Implementation

Tweaked markdown in CONTRIBUTING.md for nimble tokens.

## 🧪 Testing

Doing it live.

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or determined no changes are needed. No docs, only tests.
package.json Show resolved Hide resolved
.github/workflows/publish.yml Show resolved Hide resolved
beachball.config.js Show resolved Hide resolved
@rajsite rajsite mentioned this pull request Nov 30, 2021
1 task
@rajsite
Copy link
Member Author

rajsite commented Nov 30, 2021

Closing in favor of #209 which addresses feedback in this PR.

@rajsite rajsite closed this Nov 30, 2021
rajsite added a commit that referenced this pull request Nov 30, 2021
# Pull Request

## 🤨 Rationale

The change covers [TASK 1741883](https://ni.visualstudio.com/DevCentral/_workitems/edit/1741883) which is to switch nimble-angular to be a View Engine based build as discussed in the [ensuring library version compatibility topic](https://v12.angular.io/guide/creating-libraries#ensuring-library-version-compatibility).

This PR also addresses the feedback given in #207.

## 👩‍💻 Implementation

- Change `angular-workspace/projects/ni/nimble-angular/tsconfig.lib.prod.json` to `"enableIvy": false`.
- Modify the publish step to rebuild the package before publish.
  - During the full build angular runs ngcc in the built library package mutating the contents. The library must be rebuilt to a clean state before publish.

## 🧪 Testing

Relies on existing test infrastructure.

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or determined no changes are needed. Updated beachball workflow discussion.
@rajsite rajsite deleted the post-angular-workflow-update branch December 3, 2021 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants