Skip to content

Lock Antora dependencies - #367

Merged
ppkarwasz merged 21 commits into
mainfrom
feature/366-npm-shrinkwrap
May 19, 2025
Merged

Lock Antora dependencies#367
ppkarwasz merged 21 commits into
mainfrom
feature/366-npm-shrinkwrap

Conversation

@ppkarwasz

Copy link
Copy Markdown
Member

We use npm-shrinkwrap.json to publish a locked list of dependency versions.

Other repositories will use this file to download a locked list of dependencies.

@ppkarwaszppkarwasz moved this from To triage to In review in Log4j bug trackerApr 1, 2025
@ppkarwasz
ppkarwasz requested a review from vyApril 1, 2025 06:32
@vy

vy commented Apr 7, 2025

Copy link
Copy Markdown
Member

@ppkarwasz, mind sharing the motivation for this feature, please? I mean, what do we fix? If we don't fix anything, but improve something, what is that we improve and why do we want this improvement?

@ppkarwasz

Copy link
Copy Markdown
MemberAuthor

There are several advantages of locking the versions of the Antora dependencies:

  1. The first one is certainly security: transitive NPM dependencies will be upgraded only by Dependabot, when we release a new version.
  2. We can also save time on caching, since we can reuse an NPM workflow. This is also suggested in the deploy-site-reusable workflow:
    # We should be calculating the cache key using `package-lock.json` instead!
    # See https://stackoverflow.com/a/48524475/1278899
    # For that, `package-lock.json` needs to be committed into the repository – right now it is `.gitignore`d.
    # Once it is there, we should ideally switch from `npm i` to `npm ci`.
    # For that, we need to configure `dependabot` to update hundreds of dependencies listed in `package-lock.json`.
    # That translates to a never ending rain of `dependabot` PRs.
    # I doubt if the wasted CPU cycles worth the gain.
    key: ${{ runner.os }}-nodejs-cache-${{ hashFiles('node', 'node_modules') }}
    # `actions/cache` doesn't recommend caching `node_modules`.
    # Though none of its recipes fit our bill, since we install Node.js using `frontend-maven-plugin`.
    # See https://github.com/actions/cache/blob/main/examples.md#node---npm
    # We settle for this quick-n-dirty solution for the time being.
    path: |
    node
    node_modules

    Note: I think that there is currently a bug in the workflow since ${{ hashFiles('node', 'node_modules') }} is used before those folders are created in the mvn site step.

@vy

vy commented Apr 8, 2025

Copy link
Copy Markdown
Member

transitive NPM dependencies will be upgraded only by Dependabot

Does this mean Dependabot will try to upgrade hundreds of dependencies listed in npm-shrinkwrap.json?

This is also suggested in the deploy-site-reusable workflow

Doesn't this warrant a correction on deploy-site-reusable.yaml in this PR?

@ppkarwaszppkarwasz moved this from In review to Ready in Log4j bug trackerApr 10, 2025
@ppkarwasz

Copy link
Copy Markdown
MemberAuthor

transitive NPM dependencies will be upgraded only by Dependabot

Does this mean Dependabot will try to upgrade hundreds of dependencies listed in npm-shrinkwrap.json?

Yes, Dependabot will upgrade about a hundred NPM dependencies, but only once a month using a single PR.
See c1134f8.

This is also suggested in the deploy-site-reusable workflow

Doesn't this warrant a correction on deploy-site-reusable.yaml in this PR?

I modified the key used for the Antora cache, so that only one cache per logging-parent version is used. We don't need more caches, since node_modules should only contain JavaScript files and the node folder does not need to be cached (Node is stored in the Maven local repository).
See b1f15b1

Comment threadgithub.json Outdated
@ppkarwasz

Copy link
Copy Markdown
MemberAuthor

I modified the way node_modules are cached: since running a second actions/checkout in the same workflow deletes node_modules, the folder must be restored at the end of the workflow.

Caution

Remove the temporary entries marked with FIXME from deploy-site.yaml before merging and remove the test-site-stg-out branch.

vy
vy approved these changes May 3, 2025
Comment thread.github/workflows/deploy-site-reusable.yaml Outdated
Comment threadpom.xml Outdated
ppkarwaszand others added 2 commits May 19, 2025 20:51
@ppkarwasz
ppkarwasz merged commit 5af05ca into mainMay 19, 2025
@ppkarwasz
ppkarwasz deleted the feature/366-npm-shrinkwrap branch May 19, 2025 18:55
@github-project-automationgithub-project-automationBot moved this from Ready to Done in Log4j bug trackerMay 19, 2025
ppkarwasz added a commit that referenced this pull request May 19, 2025
This removes the temporary testing site used in PR #367.
ppkarwasz added a commit that referenced this pull request May 19, 2025
This removes the temporary testing site used in PR #367.
@ppkarwaszppkarwasz linked an issue Jun 3, 2025 that may be closed by this pull request
ppkarwasz added a commit that referenced this pull request Jun 10, 2025
ppkarwasz added a commit that referenced this pull request Jun 10, 2025
This PR updates the caching strategy for Node.js dependencies to base the cache key on the contents of `package-lock.json`, rather than the `node` or `node_modules` directories.
### Problem
Currently, caching is ineffective because:
* The cache key relies on `node` and `node_modules` directories.
* These directories are not checked into the repository and therefore don’t exist when the cache is computed.
### Solution
Update the cache key to use the contents of `package-lock.json`, which is a reliable representation of the dependency tree:
* If `package-lock.json` **is not committed**, the cache behavior remains unchanged.
* If it **is committed**, caching will function as expected, restoring and saving dependencies based on lockfile changes.
### Additional Changes
* Commit `package-lock.json` in the `logging-parent` repository to evaluate the effectiveness of this approach.
* Reverts the use of `npm-shrinkwrap.json` introduced in #367.
ppkarwasz added a commit that referenced this pull request Jun 10, 2025
This PR updates the caching strategy for Node.js dependencies to base the cache key on the contents of `package-lock.json`, rather than the `node` or `node_modules` directories.
### Problem
Currently, caching is ineffective because:
* The cache key relies on `node` and `node_modules` directories.
* These directories are not checked into the repository and therefore don’t exist when the cache is computed.
### Solution
Update the cache key to use the contents of `package-lock.json`, which is a reliable representation of the dependency tree:
* If `package-lock.json` **is not committed**, the cache behavior remains unchanged.
* If it **is committed**, caching will function as expected, restoring and saving dependencies based on lockfile changes.
### Additional Changes
* Commit `package-lock.json` in the `logging-parent` repository to evaluate the effectiveness of this approach.
* Reverts the use of `npm-shrinkwrap.json` introduced in #367.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Lock Antora dependencies

2 participants

@ppkarwasz@vy