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

Update example-basic-pnpm.yml #1291

Closed
wants to merge 1 commit into from
Closed

Conversation

tordans
Copy link

@tordans tordans commented Nov 1, 2024

Having those hints inline saves quite a bit of research.

  • Add hint how to omit setting the pnpm version explicitly
  • Add hint that use-node-version is not supported yet

- Add hint how to omit setting the pnpm version explicitly
- Add hint that `use-node-version` is not supported yet
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@cypress-app-bot
Copy link

@MikeMcC399 MikeMcC399 added tests type: enhancement New feature or request labels Nov 1, 2024
@MikeMcC399 MikeMcC399 assigned MikeMcC399 and tordans and unassigned MikeMcC399 Nov 1, 2024
@MikeMcC399
Copy link
Collaborator

@tordans

Thank you for your submission to add comments to .github/workflows/example-basic-pnpm.yml!

If you would like the Cypress.io team to consider your submission, you would need to sign the CLA agreement.

In terms of the content, the workflow does already include links to the action repositories:

The workflow comments are intended to support a minimal working example and not cover all variations. Details of the actions used can change over time and adding more detail brings in the risk that the added detail of external actions may become inaccurate over time. We rely on the external action documentation being kept up to date and for users to refer to that up to date documentation.

My preference would be to not add this detail for the above reasons. You can wait to see if the Cypress.io team also responds.

@@ -21,13 +21,17 @@ jobs:
# See https://github.com/pnpm/action-setup
- name: Install pnpm
uses: pnpm/action-setup@v4
# Optional when there is a `packageManager` field in the `package.json`.
# See https://github.com/pnpm/action-setup?tab=readme-ov-file#version
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://nodejs.org/api/corepack.html lists this feature as experimental and not suitable for production.

Stability: 1 - Experimental. The feature is not subject to semantic versioning rules. Non-backward compatible changes or removal may occur in any future release. Use of the feature is not recommended in production environments.

Copy link
Author

Choose a reason for hiding this comment

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

Interesting. However since the action has documentation on supporting it, they will still lookup the information from there, even if packageManager is not an official property anymore, right?

with:
version: 9

# See https://github.com/actions/setup-node
- name: Install Node.js
uses: actions/setup-node@v4
with:
# Note that pnpm `use-node-version` is not supported, yet
# See https://github.com/actions/setup-node/issues/1130
Copy link
Collaborator

Choose a reason for hiding this comment

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

actions/setup-node#1130 is an open enhancement request

This isn't really the right place to be listing an enhancement request for an external action, when this isn't used in the example.

Copy link
Author

Choose a reason for hiding this comment

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

My point was to link to some place that explains that there is no way to set the node version based on the config that pnpm supports. I don't see a better place where this is documented.

Copy link
Collaborator

@MikeMcC399 MikeMcC399 left a comment

Choose a reason for hiding this comment

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

Some in-line comments also added.

@tordans
Copy link
Author

tordans commented Nov 1, 2024

Some in-line comments also added.

Thanks. I understand if those code comments are not in line with what you want in your example. Feel free to close this PR in this case.

@MikeMcC399
Copy link
Collaborator

@tordans

Thanks. I understand if those code comments are not in line with what you want in your example. Feel free to close this PR in this case.

Thanks for understanding that we try to keep the documentation as lean as possible for maintainability reasons.

I'm going to close this PR now. Thank you nevertheless for your proposal!

@MikeMcC399 MikeMcC399 closed this Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants