chore: fix npmPackage update failure in CI [skip ci]#9448
Conversation
| }; | ||
| try { | ||
| const results = await replace(options) | ||
| const { replaceInFile } = await import('replace-in-file'); |
There was a problem hiding this comment.
Should we convert the whole script to ESM instead to not mix require and import in a single file?
There was a problem hiding this comment.
let us keep this for now, as it is the easiest way to keep all branches to work on TC, so that it wont block any going on things
migrating to ESM will need to update all script files here and its libarary in all supported branches, let us create a ticket, so we can do it in other PR and apply it to all branches
|
seems the [skip ci] has skipped the the build for |
|
I don't see the point. If you want to skip CI, it should skip CI. It's your responsibility to only use it when you're sure that the changes don't affect compilation, tests, etc. |
|
my point is.. those builds are marked so either we dont mark those as required or just run them when |
|
It sounds like there should be a final step / workflow that fails if any previous step fails, which should then be the only one that is required for merging. I'm not sure if "Validation / Collect tests" already satisfies that criteria. |
|
Removed the fast tests jobs from the required checks and only kept "Collect tests". Also created #9455 to improve that job into a more comprehensive "Validation results" that fails if any previous job fails. |
…2) (#9456) This PR cherry-picks changes from the original PR #9448 to branch 25.2. --- #### Original PR description > after bumping `replace-in-file` from 6.x to 8.x in previous commit (f87a9d9) > the NpmPackage update build has failed with following error > ``` > Error [ERR_REQUIRE_ESM]: require() of ES Module /opt/agent/work/1fcf94d1da2f425a/vaadin-flow-components/node_modules/replace-in-file/index.js from /opt/agent/work/1fcf94d1da2f425a/vaadin-flow-components/scripts/updateNpmVer.js not supported. > [18:30:42 ](https://bender.vaadin.com/buildConfiguration/VaadinFlowComponents_FlowComponentsUpdateNpmPackagesWebjarsVersions/889722?buildTab=log&focusLine=835&logView=flowAware&linesState=835) Instead change the require of index.js in /opt/agent/work/1fcf94d1da2f425a/vaadin-flow-components/scripts/updateNpmVer.js to a dynamic import() which is available in all CommonJS modules. > [18:30:42 ](https://bender.vaadin.com/buildConfiguration/VaadinFlowComponents_FlowComponentsUpdateNpmPackagesWebjarsVersions/889722?buildTab=log&focusLine=836&logView=flowAware&linesState=836) at Object.<anonymous> (/opt/agent/work/1fcf94d1da2f425a/vaadin-flow-components/scripts/updateNpmVer.js:14:17) { > [18:30:42 ](https://bender.vaadin.com/buildConfiguration/VaadinFlowComponents_FlowComponentsUpdateNpmPackagesWebjarsVersions/889722?buildTab=log&focusLine=837&logView=flowAware&linesState=837) code: 'ERR_REQUIRE_ESM' > ``` > > root cause: > > - `replace-in-file` became ESM-only starting with v7. > - But scripts/updateNpmVer.js:14 still loads it with CommonJS require: > const replace = require('replace-in-file'); > - Under Node 18 in CI, require() of an ESM package throws ERR_REQUIRE_ESM, so the script crashes immediately and the PR step exits with code 1. > Co-authored-by: Zhe Sun <31067185+ZheSun88@users.noreply.github.com>
…25.2) (#9472) This PR cherry-picks changes from the original PR #9455 to branch 25.2. --- #### Original PR description > The results job only failed on test failures captured in report XMLs. It did not fail when any previous job failed, for example the build, or when a test job runs into an error that didn't produce failed tests. > > This changes the result step to run unconditionally and fail on any job's failure/canceled result, so it can serve as the single required check. Renamed the job to "Validation result" to reflect the broader responsibility. > > This change was triggered by the discussion in this PR: #9448. There the required checks on "Unit tests", "Package WAR", etc. prevented the PR from merging, as with `[skip ci]` those jobs are never actually run due to `fast-tests` being skipped and never spawning them. As such it seems useful to have a single result job as required check that reports success for all previous jobs and test results. Co-authored-by: Sascha Ißbrücker <sissbruecker@vaadin.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 participants
after bumping
replace-in-filefrom 6.x to 8.x in previous commit (f87a9d9)the NpmPackage update build has failed with following error
root cause:
replace-in-filebecame ESM-only starting with v7.const replace = require('replace-in-file');