-
Notifications
You must be signed in to change notification settings - Fork 10
ci(deserializer): fix npm #451
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
Conversation
feb8a61 to
56dfc22
Compare
| npm run codegen | ||
| npm run build | ||
| cd ../dataprotector-deserializer | ||
| npm run test:prepare |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need this ?
| working-directory: "packages/dataprotector-deserializer" | ||
| install-command: | | ||
| npm ci | ||
| cd ../sdk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have make this for get the artifacts from the other build job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR restructures the CI workflow for publishing the deserializer package by replacing the old set-env job with a set-publish-version job, adding a beta tag option, and updating the publish-npm step to use version 1.4.0 of the reusable workflow with additional setup commands.
- Added a
betarelease tag alongsidelatestandnightly - Introduced a
set-publish-versionjob to calculate and output the package version - Updated
publish-npmto depend onset-publish-version, use v1.4.0 of the reusable workflow, and perform SDK build and test preparation
Comments suppressed due to low confidence (5)
.github/workflows/publish-npm-deserializer.yml:17
- [nitpick] The job is named
set-publish-versionbut also checks out code and sets up Node.js. Consider renaming this tobuild-and-set-version(or split into separatebuild-sdkandset-versionjobs) to more accurately reflect its responsibilities.
set-publish-version:
.github/workflows/publish-npm-deserializer.yml:29
- Using
==in a POSIXshconditional can be brittle on shells that don't support it; use a single=for maximum compatibility ([ ... = ... ]).
if [ "${{ github.event.inputs.tag }}" == "nightly" ]; then
.github/workflows/publish-npm-deserializer.yml:26
- You added a
betatag option for inputs but the version-setting logic only special-casesnightlyand falls back to the base version forbetaandlatest. Ifbetashould publish a pre-release (e.g.1.0.0-beta), extend the script to handle thebetatag explicitly.
if: github.event.inputs.tag == 'nightly'
.github/workflows/publish-npm-deserializer.yml:47
- The workflow now always uses
productionenvironment. Previously, staging was handled viaset-env; ensure that non-latesttags (e.g.beta,nightly) are not inadvertently deployed to a production environment.
environment: production
.github/workflows/publish-npm-deserializer.yml:50
- [nitpick] Chaining multiple
cdcommands ininstall-commandcan be fragile; consider using separate actions orworking-directoryoverrides for each step to avoid directory navigation issues.
install-command: |
This pull request updates the
.github/workflows/publish-npm-deserializer.ymlfile to restructure the CI/CD workflow by replacing theset-envjob with a newbuild-sdkjob and modifying thepublish-npmjob to align with these changes.Workflow restructuring:
set-envjob with a newbuild-sdkjob that uses a Node.js 18 container, installs dependencies, runs code generation, builds the SDK, and uploads the build artifacts (sdk-dist).publish-npmjob to depend on thebuild-sdkjob instead ofset-env. It now uses thefeat/publish-npmversion of the reusable workflow and includes additional setup commands (npm ci && npm run test:prepare).