Skip to content

chore: Add composite setup action for pnpm#6507

Merged
marikaner merged 8 commits intomainfrom
pnpm-setup
Apr 21, 2026
Merged

chore: Add composite setup action for pnpm#6507
marikaner merged 8 commits intomainfrom
pnpm-setup

Conversation

@davidkna-sap
Copy link
Copy Markdown
Member

@davidkna-sap davidkna-sap commented Apr 17, 2026

Add composite setup action for pnpm

Tested via SAP/ai-sdk-js#1749

Comment thread .github/actions/setup/action.yml Outdated
Comment thread .github/actions/setup/action.yml
Comment thread .github/actions/setup/action.yml Outdated
Comment thread .github/actions/setup/action.yml Outdated
description: 'Node.js version'
default: '22'
registry-url:
description: 'The registry URL to use for npm packages. Defaults to the SAP Artifactory mirror when npm-token is provided, otherwise npmjs.'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[q] I assume the fallback to npmjs is there for external contributors? If that is the case, why do we even have configuration for it? Apparently it is never used anyways. Also, I fear that it is too easy to forget passing the token and then you never use artifactory at all.

I didn't think this through to the end, but what do you think about checking what repo this is running on aka if: github.repository == 'SAP/cloud-sdk-js' then require a token and fail if not passed, otherwise if no token passed use npmjs.org.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is also for moving back to the main npm registry for release builds, see SAP/ai-sdk-js#1749

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense. Then, how about the following:

  • we leave both inputs registry-url and registry-token
  • by default we check if: github.repository == 'SAP/cloud-sdk-js' then require a token for artifactory and fail if not passed, otherwise if no token passed use npmjs.org
  • if a registry-url was passed on SAP/cloud-sdk-js use it as passed

Also, as this is used in ai-sdk-js as well, maybe we include both in here (now it's getting ugly) or check for the SAP org only (okish, because all projects should use SAP artifactory anyways).
What do you think?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@marikaner Doesn't this match the way it is currently implemented?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Haha, I just saw, that you already implemented it in a similar way 😊

Comment thread .github/actions/setup/action.yml Outdated
- uses: actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f # v6.3.0
with:
node-version: ${{ inputs.node-version }}
registry-url: ${{ inputs.registry-url != '' && inputs.registry-url || (inputs.npm-token != '' && 'https://common.repositories.cloud.sap/artifactory/api/npm/build.releases.npm/' || 'https://registry.npmjs.org') }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[req] Whatever we decide the logic to be here, let's put this into a separate step with proper outputs for better readability and maintainability.

@davidkna-sap davidkna-sap requested a review from marikaner April 20, 2026 08:10
Copy link
Copy Markdown
Contributor

@marikaner marikaner left a comment

Choose a reason for hiding this comment

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

I like this better. Although I have one small request still, I already approved as it should not be a blocker.

Comment thread .github/actions/setup/action.yml Outdated
Comment on lines +46 to +47
elif [[ -n "$REGISTRY_TOKEN" ]]; then
echo "url=https://common.repositories.cloud.sap/artifactory/api/npm/build.releases.npm/" >> $GITHUB_OUTPUT
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like this better now, because you have to set one of the values at least, making it safe against forgetfulness.
[pp] One possible issue though: When both are set, the URL is now overwritten. I would prefer that not to happen.

@marikaner marikaner merged commit 0ffc5c8 into main Apr 21, 2026
15 checks passed
@marikaner marikaner deleted the pnpm-setup branch April 21, 2026 08:25
davidkna-sap added a commit that referenced this pull request Apr 21, 2026
* origin/main:
  chore(deps): bump follow-redirects from 1.15.11 to 1.16.0 (#6495)
  chore: Add composite setup action for pnpm (#6507)
  fix: Avoid caching jwt if it needs to be forwarded (#6007)
  Change status from proposed to decided
  chore(deps): bump typescript-eslint from 8.58.1 to 8.58.2 (#6514)
  chore(deps): bump @typescript-eslint/parser from 8.58.1 to 8.58.2 (#6513)
  chore(deps-dev): bump @sap/cds-dk from 9.8.3 to 9.8.4 (#6512)
  chore(deps): bump fast-xml-parser from 5.5.11 to 5.6.0 (#6509)
  chore(deps-dev): bump puppeteer from 24.40.0 to 24.41.0 (#6508)
davidkna-sap added a commit that referenced this pull request Apr 21, 2026
…eckapi

* origin/main:
  chore: Refactor test imports to use @sap-cloud-sdk/test-util-internal pkg (#6467)
  chore: Add license-checker action for pnpm (#6473)
  chore(deps): bump follow-redirects from 1.15.11 to 1.16.0 (#6495)
  chore: Add composite setup action for pnpm (#6507)
  fix: Avoid caching jwt if it needs to be forwarded (#6007)
  Change status from proposed to decided
  chore(deps): bump typescript-eslint from 8.58.1 to 8.58.2 (#6514)
  chore(deps): bump @typescript-eslint/parser from 8.58.1 to 8.58.2 (#6513)
  chore(deps-dev): bump @sap/cds-dk from 9.8.3 to 9.8.4 (#6512)
  chore(deps): bump fast-xml-parser from 5.5.11 to 5.6.0 (#6509)
  chore(deps-dev): bump puppeteer from 24.40.0 to 24.41.0 (#6508)
davidkna-sap added a commit that referenced this pull request Apr 21, 2026
* origin/main:
  chore(check-public-api): Use tempdir instead of mockfs (#6468)
  chore(deps): bump prettier from 3.8.2 to 3.8.3 (#6510)
  chore: Refactor test imports to use @sap-cloud-sdk/test-util-internal pkg (#6467)
  chore: Add license-checker action for pnpm (#6473)
  chore(deps): bump follow-redirects from 1.15.11 to 1.16.0 (#6495)
  chore: Add composite setup action for pnpm (#6507)
  fix: Avoid caching jwt if it needs to be forwarded (#6007)
  Change status from proposed to decided
  chore(deps): bump typescript-eslint from 8.58.1 to 8.58.2 (#6514)
  chore(deps): bump @typescript-eslint/parser from 8.58.1 to 8.58.2 (#6513)
  chore(deps-dev): bump @sap/cds-dk from 9.8.3 to 9.8.4 (#6512)
  chore(deps): bump fast-xml-parser from 5.5.11 to 5.6.0 (#6509)
  chore(deps-dev): bump puppeteer from 24.40.0 to 24.41.0 (#6508)
davidkna-sap added a commit that referenced this pull request Apr 21, 2026
* origin/main:
  chore(check-public-api): Use tempdir instead of mockfs (#6468)
  chore(deps): bump prettier from 3.8.2 to 3.8.3 (#6510)
  chore: Refactor test imports to use @sap-cloud-sdk/test-util-internal pkg (#6467)
  chore: Add license-checker action for pnpm (#6473)
  chore(deps): bump follow-redirects from 1.15.11 to 1.16.0 (#6495)
  chore: Add composite setup action for pnpm (#6507)
  fix: Avoid caching jwt if it needs to be forwarded (#6007)
  Change status from proposed to decided
  chore(deps): bump typescript-eslint from 8.58.1 to 8.58.2 (#6514)
  chore(deps): bump @typescript-eslint/parser from 8.58.1 to 8.58.2 (#6513)
  chore(deps-dev): bump @sap/cds-dk from 9.8.3 to 9.8.4 (#6512)
  chore(deps): bump fast-xml-parser from 5.5.11 to 5.6.0 (#6509)
  chore(deps-dev): bump puppeteer from 24.40.0 to 24.41.0 (#6508)
  chore(deps-dev): bump typedoc from 0.28.18 to 0.28.19 (#6505)
davidkna-sap added a commit that referenced this pull request Apr 22, 2026
…e-and-write-changelogs

* origin/main: (36 commits)
  chore: Replace mock-fs with memfs/unionfs for fs mocking (#6470)
  chore(deps-dev): bump @changesets/cli from 2.30.0 to 2.31.0 (#6515)
  chore(deps): bump bignumber.js from 10.0.2 to 11.0.0 (#6511)
  chore(deps): bump @changesets/get-release-plan from 4.0.15 to 4.0.16 (#6518)
  chore(check-public-api): Use tempdir instead of mockfs (#6468)
  chore(deps): bump prettier from 3.8.2 to 3.8.3 (#6510)
  chore: Refactor test imports to use @sap-cloud-sdk/test-util-internal pkg (#6467)
  chore: Add license-checker action for pnpm (#6473)
  chore(deps): bump follow-redirects from 1.15.11 to 1.16.0 (#6495)
  chore: Add composite setup action for pnpm (#6507)
  fix: Avoid caching jwt if it needs to be forwarded (#6007)
  Change status from proposed to decided
  chore(deps): bump typescript-eslint from 8.58.1 to 8.58.2 (#6514)
  chore(deps): bump @typescript-eslint/parser from 8.58.1 to 8.58.2 (#6513)
  chore(deps-dev): bump @sap/cds-dk from 9.8.3 to 9.8.4 (#6512)
  chore(deps): bump fast-xml-parser from 5.5.11 to 5.6.0 (#6509)
  chore(deps-dev): bump puppeteer from 24.40.0 to 24.41.0 (#6508)
  chore(deps-dev): bump typedoc from 0.28.18 to 0.28.19 (#6505)
  chore(deps): bump ts-morph from 27.0.2 to 28.0.0 (#6506)
  chore(deps-dev): bump globals from 17.4.0 to 17.5.0 (#6504)
  ...
davidkna-sap added a commit that referenced this pull request Apr 22, 2026
* origin/main: (27 commits)
  chore: Replace mock-fs with memfs/unionfs for fs mocking (#6470)
  chore(deps-dev): bump @changesets/cli from 2.30.0 to 2.31.0 (#6515)
  chore(deps): bump bignumber.js from 10.0.2 to 11.0.0 (#6511)
  chore(deps): bump @changesets/get-release-plan from 4.0.15 to 4.0.16 (#6518)
  chore(check-public-api): Use tempdir instead of mockfs (#6468)
  chore(deps): bump prettier from 3.8.2 to 3.8.3 (#6510)
  chore: Refactor test imports to use @sap-cloud-sdk/test-util-internal pkg (#6467)
  chore: Add license-checker action for pnpm (#6473)
  chore(deps): bump follow-redirects from 1.15.11 to 1.16.0 (#6495)
  chore: Add composite setup action for pnpm (#6507)
  fix: Avoid caching jwt if it needs to be forwarded (#6007)
  Change status from proposed to decided
  chore(deps): bump typescript-eslint from 8.58.1 to 8.58.2 (#6514)
  chore(deps): bump @typescript-eslint/parser from 8.58.1 to 8.58.2 (#6513)
  chore(deps-dev): bump @sap/cds-dk from 9.8.3 to 9.8.4 (#6512)
  chore(deps): bump fast-xml-parser from 5.5.11 to 5.6.0 (#6509)
  chore(deps-dev): bump puppeteer from 24.40.0 to 24.41.0 (#6508)
  chore(deps-dev): bump typedoc from 0.28.18 to 0.28.19 (#6505)
  chore(deps): bump ts-morph from 27.0.2 to 28.0.0 (#6506)
  chore(deps-dev): bump globals from 17.4.0 to 17.5.0 (#6504)
  ...
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