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

feat(release): subproject releases #3208

Merged
merged 65 commits into from
Jan 5, 2024

Conversation

dkershner6
Copy link
Contributor

@dkershner6 dkershner6 commented Dec 28, 2023

Resolves #3172

This is non-breaking, but some opinions made:

  • Releases for each project should be a separate workflow, named after the project in the case of a subproject
  • For the base release workflow job (creation of artifact), the default working-directory should be set to the relative outdir of the subproject, with an exception made for the install command, which should be run from the topLevelParent of the subproject outdir.
  • The folder that the artifact is published from, for a subproject, should be the subproject's outdir + artifactDirectory.
  • Perhaps we should break TaskWorkflow, but I just decomposed it.
  • Settings - Nothing has changed default-wise, and as such, in order to release subprojects, you must manually set github to true (@mrgrain comments) and release to true.

Snapshots have changed, but it has been verified that the default release behavior for NON-subprojects has remained identical.

I attempted to test this on a monorepo, but the package linking process combined with projen's automated nature proved too difficult, but based on the unit tests, I believe it would satisfy.

Open to more testing ideas, but wanted to ensure the opinions formed are the right ones.

I considered writing a recursive releaseTagPrefix conflict checker, but it seemed outside the scope, and should be another request if desired.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dkershner6
Copy link
Contributor Author

In order to test this in a more realistic way, I was able to add a project to projen with the following code.

new javascript.NodeProject({
  parent: project,
  name: "my-subproject",
  defaultReleaseBranch: "main",
  outdir: "packages/my-subproject",
  release: true,
  github: true,
  minNodeVersion: "16.0.0",
  workflowNodeVersion: "18.14.0",
  releaseTagPrefix: "my-subproject@",
  releaseToNpm: true,
});

This produced the following workflow:

# ~~ Generated by projen. To modify, edit .projenrc.js and run "/bin/bash ./projen.bash".

name: release_my-subproject
on:
  push:
    branches:
      - main
  workflow_dispatch: {}
jobs:
  release:
    runs-on: ubuntu-latest
    permissions:
      contents: write
    outputs:
      latest_commit: ${{ steps.git_remote.outputs.latest_commit }}
    env:
      CI: "true"
    defaults:
      run:
        working-directory: ./packages/my-subproject
    steps:
      - name: Checkout
        uses: actions/checkout@v3
        with:
          fetch-depth: 0
      - name: Set git identity
        run: |-
          git config user.name "github-actions"
          git config user.email "github-actions@github.com"
      - name: Setup Node.js
        uses: actions/setup-node@v3
        with:
          node-version: 18.14.0
      - name: Install dependencies
        run: yarn install --check-files --frozen-lockfile
        working-directory: .
      - name: release
        run: npx projen release
      - name: Check for new commits
        id: git_remote
        run: echo "latest_commit=$(git ls-remote origin -h ${{ github.ref }} | cut -f1)" >> $GITHUB_OUTPUT
      - name: Backup artifact permissions
        if: ${{ steps.git_remote.outputs.latest_commit == github.sha }}
        run: cd dist && getfacl -R . > permissions-backup.acl
        continue-on-error: true
      - name: Upload artifact
        if: ${{ steps.git_remote.outputs.latest_commit == github.sha }}
        uses: actions/upload-artifact@v3
        with:
          name: build-artifact
          path: packages/my-subproject/dist
  release_github:
    name: Publish to GitHub Releases
    needs: release
    runs-on: ubuntu-latest
    permissions:
      contents: write
    if: needs.release.outputs.latest_commit == github.sha
    steps:
      - uses: actions/setup-node@v3
        with:
          node-version: 18.14.0
      - name: Download build artifacts
        uses: actions/download-artifact@v3
        with:
          name: build-artifact
          path: dist
      - name: Restore build artifact permissions
        run: cd dist && setfacl --restore=permissions-backup.acl
        continue-on-error: true
      - name: Release
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
          GITHUB_REPOSITORY: ${{ github.repository }}
          GITHUB_REF: ${{ github.ref }}
        run: errout=$(mktemp); gh release create $(cat dist/releasetag.txt) -R $GITHUB_REPOSITORY -F dist/changelog.md -t $(cat dist/releasetag.txt) --target $GITHUB_REF 2> $errout && true; exitcode=$?; if [ $exitcode -ne 0 ] && ! grep -q "Release.tag_name already exists" $errout; then cat $errout; exit $exitcode; fi
  release_npm:
    name: Publish to npm
    needs: release
    runs-on: ubuntu-latest
    permissions:
      contents: read
    if: needs.release.outputs.latest_commit == github.sha
    steps:
      - uses: actions/setup-node@v3
        with:
          node-version: 18.14.0
      - name: Download build artifacts
        uses: actions/download-artifact@v3
        with:
          name: build-artifact
          path: dist
      - name: Restore build artifact permissions
        run: cd dist && setfacl --restore=permissions-backup.acl
        continue-on-error: true
      - name: Release
        env:
          NPM_DIST_TAG: latest
          NPM_REGISTRY: registry.npmjs.org
          NPM_TOKEN: ${{ secrets.NPM_TOKEN }}
        run: npx -p publib@latest publib-npm

When I compare this against my own monorepo and manual code, it looks identical other than naming and that I use PNPM. The monorepo publish is confirmed working.

@codecov-commenter
Copy link

codecov-commenter commented Dec 28, 2023

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (ad69f80) 96.38% compared to head (5ecccb6) 96.38%.

Files Patch % Lines
src/github/task-workflow-job.ts 91.28% 21 Missing ⚠️
src/project.ts 85.71% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3208      +/-   ##
==========================================
- Coverage   96.38%   96.38%   -0.01%     
==========================================
  Files         189      191       +2     
  Lines       35910    36172     +262     
  Branches     3332     3363      +31     
==========================================
+ Hits        34612    34863     +251     
- Misses       1298     1309      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/project.ts Outdated Show resolved Hide resolved
src/project.ts Outdated Show resolved Hide resolved
src/release/release.ts Outdated Show resolved Hide resolved
src/release/release.ts Outdated Show resolved Hide resolved
src/release/release.ts Outdated Show resolved Hide resolved
@mrgrain mrgrain changed the title feat: Subproject Releases feat: subproject releases Dec 29, 2023
@dkershner6
Copy link
Contributor Author

My message came across as an attempt to push, it was actually just meant to say "I am done with your most recent review". Apologies.

Oh no, didn't read it that way at all. My message was really just letting you know that you will get quicker results if you keep changes to the API surface tight. As much I as appreciate good code and refactoring, it inevitably makes everything a bit slower.

Asking me to turn my head is a big ask!

I hear you, but I also think this was very educational for me in my assessment of whether this lib can serve our needs and to what certain things are and how they should be used, so the time was not wasted (at least for me), and now we have slightly cleaner code!

@dkershner6
Copy link
Contributor Author

Might be my words. I'm not a native English speaker.

It isn't, I would've never known unless you said something.

@dkershner6
Copy link
Contributor Author

OK, @mrgrain , I handled or replied to all comments.

Note that I had to handle one differently than either of us expected, but it is resolved: #3208 (comment)

Essentially, we were erroring for reasons that no longer make sense. Now we just do nothing, as per normal, single project, behavior.

Copy link
Contributor Author

@dkershner6 dkershner6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

src/github/task-workflow-job.ts Show resolved Hide resolved
@mrgrain
Copy link
Contributor

mrgrain commented Jan 5, 2024

@dkershner6 Out of interest, have you read this yet? https://projen.io/constructs.html
Either way, does that help you understanding what scope is or is it missing something?

@mrgrain
Copy link
Contributor

mrgrain commented Jan 5, 2024

@dkershner6 Pushed a couple more changes. I am happy with this now. Are you okay for me to merge?

@dkershner6
Copy link
Contributor Author

@dkershner6 Out of interest, have you read this yet? https://projen.io/constructs.html Either way, does that help you understanding what scope is or is it missing something?

I have, and I have used em quite a bit outside this lib, and am generally familiar from CDK, but that almost would make me complacent on the scope parameter, figuring it to be mostly about context and not that I specifically needed to target the subproject.

I mean, the chances of something going wrong are pretty small regardless, but it is worth telling folks, imo.

@dkershner6
Copy link
Contributor Author

@dkershner6 Pushed a couple more changes. I am happy with this now. Are you okay for me to merge?

I am. Thanks for this, was a great experience and I love this feature!

@mrgrain
Copy link
Contributor

mrgrain commented Jan 5, 2024

@dkershner6 Out of interest, have you read this yet? https://projen.io/constructs.html Either way, does that help you understanding what scope is or is it missing something?

I have, and I have used em quite a bit outside this lib, and am generally familiar from CDK, but that almost would make me complacent on the scope parameter, figuring it to be mostly about context and not that I specifically needed to target the subproject.

I mean, the chances of something going wrong are pretty small regardless, but it is worth telling folks, imo.

Oh yeah, I was just curious. And more docs are always nice. In projen the biggest win is direct access to the project. Which I guess is kind of like context.

@mrgrain
Copy link
Contributor

mrgrain commented Jan 5, 2024

@dkershner6 Pushed a couple more changes. I am happy with this now. Are you okay for me to merge?

I am. Thanks for this, was a great experience and I love this feature!

:shipit:

@mergify mergify bot merged commit e2b393d into projen:main Jan 5, 2024
14 checks passed
@dkershner6
Copy link
Contributor Author

dkershner6 commented Jan 5, 2024

@dkershner6 Out of interest, have you read this yet? https://projen.io/constructs.html Either way, does that help you understanding what scope is or is it missing something?

I have, and I have used em quite a bit outside this lib, and am generally familiar from CDK, but that almost would make me complacent on the scope parameter, figuring it to be mostly about context and not that I specifically needed to target the subproject.
I mean, the chances of something going wrong are pretty small regardless, but it is worth telling folks, imo.

Oh yeah, I was just curious. And more docs are always nice. In projen the biggest win is direct access to the project. Which I guess is kind of like context.

Yes, very similar in benefit and use case of Cdk.Stack.of. I will freely admit that it takes a bit of getting used to, but that might be something about convention, like a Java dev would immediately grasp it. Either way, after getting used to, very useful.

I make pretty much everything in my libs a Component, just in case I need something.

@dkershner6 dkershner6 deleted the DAK/release-subprojects branch January 5, 2024 23:58
mergify bot pushed a commit that referenced this pull request Mar 25, 2024
Creates a working `build` workflow specific to a sub-project when that project has `buildWorkflow` set to `true`.

Based on #3208. So most of the same decisions were made. The `install` command is always run in the root directory same as the `release` workflow. But based on discussion in #3304, we may want to make that configurable. I could make that change in this PR for `build`. Or just address it in a followup PR for both `build` and `release`.
I added a `buildWorkflowName` option to `NodeProject` to support customizing each build workflow name; but it still defaults to `build` or `build_{package-name}` for sub-projects.

I've successfully tested this with my own monorepo and added a few tests.

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
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.

Support subproject releases
3 participants