[JENKINS-73744] npm scripts lint:fix do not work

[scherler]

See JENKINS-73744.

Testing done

Proposed changelog entries

• human-readable text

Proposed upgrade guidelines

N/A

Submitter checklist

Beta Give feedback

1. The Jira issue, if it exists, is well-described.
2. The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
3. There is automated testing or an explanation as to why this change has no tests.
4. New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
5. New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
6. New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
7. For dependency updates, there are links to external changelogs and, if possible, full differentials.
8. For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Beta Give feedback

1. There are at least two (2) approvals for the pull request and no outstanding requests for change.
2. Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
3. Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
4. Proper changelog labels are set so that the changelog can be generated automatically.
5. If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
6. If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[timja]

Please update the CONTRIBUTING guide: https://github.com/jenkinsci/jenkins/blob/master/CONTRIBUTING.md#running-the-yarn-frontend-build

Changes look good although I would like to run locally before approving

[janfaracik]

Working locally for me +1 - ran a dev server and tried linting.

I think it makes more sense for first time contributors to have package.json in the root too.

[scherler]

scherler#75 I created a PR for the profiles only but I am not sure if that is what @timja had in mind

[timja]

Thanks! I tested locally and linting looks to be fine for me.

[timja]

/label ready-for-merge

---

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

[github-actions]

Please take a moment and address the merge conflicts of your pull request. Thanks!

[MarkEWaite]

I found that with the most recent changes and a merge of the latest master branch, I needed to add the downloaded versions of Node and Yarn to my path. I did that with the command:

export PATH=$PWD/node:$PWD/node/yarn/dist/bin:$PATH

I have no skills in front-end development, so that may indicate that I'm making some obvious mistake, but when I tried to run yarn without adding it to the PATH, my Linux shell reported command not found.

Once the PATH was updated, I confirmed that yarn lint:fix worked for me when I intentionally modified a JavaScript file to have a formatting error.

[timja]

That’s expected, maven doesn’t add the downloaded yarn and node to the PATH.

[MarkEWaite]

That’s expected, maven doesn’t add the downloaded yarn and node to the PATH.

Thanks! I think that means the CONTRIBUTING doc should be updated to include the instructions to set the PATH for the new location of node and yarn. I pushed that change as 8e868b5

[daniel-beck]

In what way does this address JENKINS-73744?

[timja]

@scherler are you happy with that change?

I and most other people doing frontend development are unlikely to use that approach as we'll use what we have locally installed so we don't have to modify our PATH for one project.

Node will read what's in the package.json and automatically load the right version of yarn anyway once you've run corepack enable

[MarkEWaite]

I and most other people doing frontend development are unlikely to use that approach as we'll use what we have locally installed so we don't have to modify our PATH for one project.

Would it be a workable compromise for the less experienced (like me) if there were a section that described the PATH setting without including a link in the later sections to those instructions?

[scherler]

@MarkEWaite @timja I will update doc to point out the path facts
^ just saw that you actually did. As Tim said as a frontend dev you would have yarn and node in your path anyway but I guess you than just ignore the note.

[scherler]

@daniel-beck I am not sure about the question since the change is addressing the problem and it is now possible to use lint:fix as expected and without having to add the node_modules to the path. This is because moving the node/npm infrastructure to the root folder is installing all node_modules there and next to the config file which formally had failed.

[basil]

@daniel-beck I am not sure about the question since the change is addressing the problem and it is now possible to use lint:fix as expected and without having to add the node_modules to the path.

Then why does CONTRIBUTING.md still talk about adding to the PATH?

[basil]

And why was the node.version configuration in .github/renovate.json not updated to adjust to the new file path?

[scherler]

@basil Actually @MarkEWaite added that note, you only need to add node/yarn to your path if you want to use yarn ... in your jenkins setup without having yarn globally installed on your box. In my case I do not have to do this anymore since I have yarn installed on a global level. Hence #9718 (comment) you would have yarn and node in your path anyway but I guess you than just ignore the note. I was not aware of that config .github/renovate.json but we neither changed the version, but I think you mean

      "fileMatch": [
        "war/pom.xml"
      ],

I will fill a follow-up PR if you do not beat me to it.

[basil]

@scherler The documentation should always reflect what is supported by the project, no more and no less.

• If usage with PATH modification is supported and usage without PATH modification is unsupported, then this PR should be reverted.
• If usage with PATH modification is unsupported and usage without PATH modification is supported, then the documentation should be updated to remove references to PATH modification.
• If usage with or without PATH modification is supported, the documentation should describe both usages.

In any case, this PR (as it was integrated) is logically inconsistent.

[scherler]

I will follow up as well to state in the docu what I explained earlier that you only have to add the path if yarn is not installed globally.

you only have to add the following to your path if you do not have it already (due to a global install of node/yarn)

however it will not break anything if you follow that instruction on a system that already has node/yarn installed, so not really sharing your opinion of logically inconsistent

[scherler]

@basil #9757

[basil]

so not really sharing your opinion of logically inconsistent

Is there any particular reason to use monospaced text here for the phrase "logically inconsistent"? Did you mean to use quotation marks?

[basil]

Why was this PR merged when the mandatory Testing Done section was blank?

[MarkEWaite]

I should have added notes in the PR description on the testing that I did in the comment. Tim noted in his comment that he tested locally, and Jan noted in his comment that he tested locally.

However, I did not test the Maven profile and should have done that.