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(#9362): publish cht-deploy to npm on version change #9363

Closed
wants to merge 13 commits into from

Conversation

henokgetachew
Copy link
Contributor

@henokgetachew henokgetachew commented Aug 21, 2024

Description

Publishes to npm on version change in cht-deploy package.json (Under scripts/deploy/). The version change has to happen manually and no automated commits to github happens. This was done on purpose, to avoid triggering the build process. It runs the cht-deploy tests to make sure they pass before publishing.

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.

Compose URLs

If Build CI hasn't passed, these may 404:

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

A couple of questions inline.

I'm assuming you haven't tested this at all, because it only runs on master? What do you think about tweaking it slightly so we can run it in this branch and check if it actually works and then switch back to master before merging?

For example, it would be good to check that, a) it actually builds and publishes correctly, and 2) it doesn't publish if it's run a second time and the version hasn't changed.

Otherwise we might end up cluttering up the commit log trying to get this to work in master.

.github/workflows/npm-publish-cht-deploy.yml Outdated Show resolved Hide resolved
Comment on lines 29 to 34
- name: Publish to npm
uses: JS-DevTools/npm-publish@v3
with:
token: ${{ secrets.NPM_TOKEN }}
access: public
package: ./scripts/deploy/package.json
Copy link
Member

Choose a reason for hiding this comment

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

In cht-conf we use semantic release to do this and it would be great to use the same in both places, but that library is designed to also update the version and generate release notes so maybe it's overkill. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, semantic release is cool and more comprehensive. I liked the automated changelog.md. I was assuming that it would try to include all the CHT related commits in the release notes. That may not be the case.. If that's not the case, there is a commits analyzer to filter out commits that don't have anything to do with cht-deploy. I'll give it a look.

@henokgetachew
Copy link
Contributor Author

Thanks @garethbowen!

Current state of things:

  • We're using semantic-release
  • The workflow triggers only on changes to the scripts/deploy/** path.
  • If the branch is master, it doesn't run the tests.
  • If the branch is any other branch, it runs the tests and it makes a pre-release to npm

We need to configure secrets - I don't have those rights on this repo:

          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
          NPM_TOKEN: ${{ secrets.NPM_TOKEN }}

That's what's remaining to see this in action.

Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

Nice one! A couple changes suggested, and I'll look at getting the tokens added.

}
],
"plugins": [
"@semantic-release/commit-analyzer",
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this won't work as expected because it'll get the changelog for all commits on the master branch, not just commits that impact this directory. What do you think?

run: npm ci

- name: Run tests
if: github.ref != 'refs/heads/master'
Copy link
Member

Choose a reason for hiding this comment

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

Once we merge this check should be at the top level instead of here because the entire publish job should not run if not on master, right?

@henokgetachew
Copy link
Contributor Author

Closing this in favor of moving cht-deploy to its own repo.

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.

2 participants