Repository navigation
CLP-1095 Migrate Orchestrator downloads to the JFrog Edge node for SonarJS - #8062
Conversation
hashicorp-vault-sonar-prod
Bot
changed the title
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
878d73d to
56f5b17
Compare
6259e36 to
4dcd8c5
Compare
97f76b9 to
bc69b37
Compare
a758c97 to
f0c4796
Compare
|
@guillaume-dequenne I'm assigning this to Victor. I think he's a better choice for this repo as this PR is not super straight forward. |
vdiez
left a comment
vdiez
left a comment
There was a problem hiding this comment.
I agree with using JFrog Edge for CI dependency resolution and Orchestrator downloads, while keeping deployment/publishing/promotion on SaaS. I am requesting changes because the committed lockfile now makes public source builds depend on our internal Edge registry. Public users must continue to use npmjs by default, without SonarSource credentials or registry workarounds.
Please make the following changes before merging.
-
Keep the committed root lockfile on npmjs.
Remove the rewrite of the root
package-lock.jsontohttps://repox-internal.dev.sonar.build/artifactory/api/npm/npm/. Normalize the public npm dependencies'resolvedURLs tohttps://registry.npmjs.org/, including any remaining SaaS Repox URLs inherited from the base branch; simply reverting the Edge rewrite would leave those inherited URLs behind. Preserve the dependency versions and integrity hashes.The current head contains 1,133 Edge tarball URLs. A public user running
npm ciwill attempt to fetch those URLs. Setting--registry=https://registry.npmjs.org/does not redirect an arbitrary internal host: npm's defaultreplace-registry-host=npmjsreplaces npmjs URLs with the configured registry, not Edge URLs with npmjs. Thus authenticated internal CI passing does not establish that public builds still work. -
Select Edge through CI configuration while preserving the npmjs defaults in the repository.
Keep
.npmrcfree of an internal registry requirement. Keep both public-registry overrides in.github/renovate.json:npmrc: "registry=https://registry.npmjs.org/\n"for npm operations/lockfile generation, and the npm datasourceregistryUrls: ["https://registry.npmjs.org/"]override for dependency lookups. These intentionally override the shared Repox preset and help keep dependency updates usable by public contributors.In the CI jobs that resolve through Edge, configure the Edge registry and its scoped authentication, and retain
replace-registry-host=npmjsbehavior so npmjs lockfile URLs are fetched through Edge at install time. This needs to work for both the sharedconfig-npmpath on Linux and the explicit npm configuration on Windows. Explicitly setting the option in CI is fine; if relying on the shared action/default, verify that behavior on a clean runner.The property originally landed in #8005 with
repox.jfrog.ioas its value. #8090, merged on October 2, changed it tonpmjswith the comment "Redirect public lockfile URLs to the configured internal registry." Preserve that public-lockfile/CI-registry separation when replacing the local action. A SaaS URL bypassing Edge is a reason to normalize that URL to npmjs, rather than commit Edge URLs for all consumers. -
Remove the Edge-specific release workaround and correct the documentation.
Once the lockfile uses npmjs URLs, remove the added
npm config set replace-registry-host repox-internal.dev.sonar.buildfromrelease_eslint_plugin.yml. Keep the intended SaaS resolve/publish/promote configuration for the release workflow, and verify that its install still resolves through the intended registry. With public lockfile URLs, the release no longer needs a workaround for an internal host committed by this PR.Update
docs/CI.mdand the PR description to explain that committed lockfile URLs and Renovate remain on npmjs, while CI selects Edge and redirects npmjs tarball URLs at runtime. Remove the instruction to keep new lockfile entries on Edge and the description of the release rewriting Edge lockfile URLs. -
Describe this as an explicit extension of the accepted #8005 scope.
In the answer to my initial review of #8005, the agreed acceptance criteria were that self-hosted/WarpBuild
mvn deployandnpm cidependency resolution use Edge, publishing/promotion remain on SaaS, and GitHub-hosted jobs remain on SaaS. A one-off Maven setup call using SaaS was explicitly acceptable. I approved that scope.It is reasonable for this PR to extend the migration. Please distinguish the new goals from the behavior already delivered: migrate Orchestrator's direct downloads (which use
ARTIFACTORY_URL, independently of the Maven mirror), move the Maven setup calls to Edge, remove the remaining ESLint build/test install exceptions that explicitly use SaaS, and replace federated SaaS reader tokens/local waiters with Edge-issued credentials/shared configuration. Explain any runner paths that now use Edge where #8005 kept SaaS, and document their reachability/validation. Keep the explicit SaaS deployment override and the SaaS publishing/promotion paths.Also update the Fast QA description:
sonar-scanner-integration-tester1.3.0.1396 is already present on master through #8092. It can be documented as a prerequisite already merged, rather than a change introduced by this PR. -
Validate the revised public-lockfile configuration before merging.
The linked validation run did execute
npm cisuccessfully on both Linux and Windows with a cold cache, which addresses the earlier concern about exercising the miss path. However, that validation used the Edge lockfile and must be repeated for the revised public lockfile.Please provide evidence that a clean public installation with npmjs as the registry and no SonarSource credentials succeeds; that Linux and Windows CI cache-miss installs fetch through Edge using npmjs lockfile URLs; and that the ESLint build/test installs and Orchestrator downloads use Edge. Verify that the release install and deploy/publish/promote paths still target SaaS. Use a fresh/isolated cache key or equivalent verification where needed; broad cache eviction is unnecessary.
The latest head's Build run also has a failing Ubuntu Node 24 QA job with four SonarLint condition timeouts. I have not established that these are caused by this PR. Please rerun the failed job and investigate if it persists, and update the validation section with results for the final head.
With these changes, we can finish the CI migration and simplify its authentication without requiring public users to access SonarSource infrastructure.
f0c4d35 to
590174b
Compare
b917dff to
8d39acd
Compare
vdiez
left a comment
vdiez
left a comment
There was a problem hiding this comment.
Thanks for addressing the requested changes. Public npmjs lockfile URLs and Renovate defaults are preserved, CI selects Edge at install time, and GitHub-hosted release configuration explicitly stays on SaaS. We can simplify the npm authentication setup in a separate cleanup refactor. Latest-head CI is still pending; merge once the required checks pass.
8d39acd to
cf66ad9
Compare
Code Review ✅ Approved 11 closed / 11 findings🟡 Medium risk · Routing npm, Maven, and Orchestrator downloads through JFrog Edge changes CI dependency resolution; routing or credential issues could block builds. Routes Orchestrator downloads, Maven setup, and ESLint build/test installs through JFrog Edge on self-hosted and WarpBuild runners, with public npm lockfile URLs preserved. Resolved the private federation-wait action, unbounded probe curl, SaaS auth fallback, unmerged config-npm pin, PR description mismatch, lockfile and release-workflow breakages, cache-step config text, and the jfrog-setup-wrapper SHA pin. Two wont-fix items remain closed without a fix: the 2.2.0 pin versus floating ✅ 11 closed✅ Bug: Public SonarJS repo can't resolve the private federation-wait action
✅ Edge Case: Probe curl has no timeout, so the 2-minute wait can run far longer
✅ Bug: SaaS
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
|
4 participants
Part of CLP-918.
Summary
Extends #8005 by routing Orchestrator's direct downloads, Maven setup calls, and the remaining ESLint build/test installs through JFrog Edge on self-hosted and WarpBuild runners. Shared configuration fetches Edge-issued reader tokens and replaces local federation waiters.
.npmrcand both Renovate npmjs overrides retain public defaults.NPM_CONFIG_REPLACE_REGISTRY_HOST=npmjsand selects Edge at install time. Linux and ESLint jobs useconfig-npm@v2; Windows configures npm directly with the Edge reader token because mise has no jfrog-cli backend there.config-maven@v2supplies the Edge mirror and Orchestrator'sARTIFACTORY_URL/reader credentials.mvn deployexplicitly setsARTIFACTORY_URLto SaaS while retaining the Edge resolve mirror. Deployment and promotion retain SaaS configuration.jfrog-setup-wrapperpin. It needs no Edge lockfile rewrite. The release-style validation also explicitly selects SaaS, so neither depends on the shared action’s changing default.Fast QA tester 1.3.0.1396, bundling Orchestrator 6.4.3, is a prerequisite already merged in #8092. The
ci-github-actions@v2references are moving major-version references, not release pins.Validation
Final revision:
8d39acd9d. PR Build and isolated registry validation are running. Earlier validation passed, including Node 24 SonarLint, Fast QA, Windows QA, and ruling; the evidence below will be refreshed when the final runs complete.repox.jfrog.io; normal PR promotion passed on SaaS. Release publish/promote configuration is unchangedThe opt-in
workflow_dispatchinputvalidate-registry-routing=trueuses isolated npm/Orchestrator cache keys, logs npm HTTP requests, and checks public/SaaS installs. It skips branch analysis and feature-branch promotion; normal PR promotion is unchanged. No shared caches were evicted and the release workflow was not triggered.Lockfile semantic comparison, YAML parsing, and
git diff --checkpass. Actionlint reports no new findings compared with master.The PR SonarQube quality gate passes with no new violations or dependency risks. The manual branch analysis exposes the existing overall licensing gate failure (severity 20, limit 19), also present on master; this PR changes no dependency versions. Schedule-only Alpine and DEV QA paths remain outside this validation coverage.