Skip to content

fixing typo on README, improving CI checks - #298

Closed
chargio wants to merge 3 commits into
trento-project:mainfrom
chargio:small_fixes
Closed

chargio wants to merge 3 commits into
trento-project:mainfrom
chargio:small_fixes

Conversation

@chargio

@chargio chargio commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Improve docker use in docker compose and CI workflows to make sure that the node versions used are in sync.

Description

A fix to make sure that the version used in the containers and CI is the same version of node used for development, comparing the version of .tool-versions and the ones in the dockerfiles.
Defined the version in a versions.yaml file so it can be reused in every container workflow

Fixes #

How was this tested?

Manual tests and automated tests

Documentation changes

Yes / No

Additional information

@chargio
chargio requested review from a team and eromanova97 and a lite review from Copilot September 3, 2026 15:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new check-node-version.sh contains a shell-quoting bug that will break execution, and the CI path filters can miss Node version bumps by not including .tool-versions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR centralizes the Node.js container image/version in a single versions.yml file and wires local Docker Compose + GitHub Actions workflows to consume and validate that pin, reducing drift between development and CI environments.

Changes:

  • Added versions.yml as the shared source of truth for the Node.js container image tag.
  • Introduced scripts/check-node-version.sh and integrated it into Docker Compose and CI workflows to verify version consistency.
  • Updated documentation (README) and adjusted workflows/compose to use the pinned image; removed trento-docs-site-ui/.nvmrc.
File summaries
File Description
versions.yml Defines the pinned Node.js container image/tag used by Compose and CI.
trento-docs-site-ui/.nvmrc Removes nvm pin in favor of .tool-versions/container pin.
scripts/check-node-version.sh New version-consistency checker and image/tag utilities for CI/Compose.
README.adoc Fixes a documentation URL and documents the new Node.js pin/check workflow.
docker-compose.yml Imports the pinned image via extends and runs the version check at service start.
.github/workflows/extension-tests.yaml Reads the pinned image from versions.yml and validates Node version in CI.
.github/workflows/build-page.yaml Same as above for the Pages build workflow.
Review details

Suppressed comments (1)

.github/workflows/extension-tests.yaml:25

  • Same as the push filter: add .tool-versions so PRs that update the Node pin can’t accidentally bypass this workflow when they touch only the toolchain version file.
      - 'trento-docs-site/package.json'
      - 'trento-docs-site/package-lock.json'
      - '.github/workflows/extension-tests.yaml'
      - 'versions.yml'
      - 'scripts/**'
  • Files reviewed: 7/7 changed files
  • Comments generated: 5
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread scripts/check-node-version.sh
Comment thread .github/workflows/build-page.yaml
Comment thread .github/workflows/extension-tests.yaml
Comment thread scripts/check-node-version.sh Outdated
Comment thread versions.yml Outdated
@chargio chargio added dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation labels Sep 7, 2026
…ompose does not support ARG interpolation, we add a check to make sure everything is aligned
@chargio
chargio marked this pull request as ready for review September 7, 2026 11:06
@chargio
chargio requested a lite review from Copilot September 7, 2026 11:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Some newly added documentation comments overstate guarantees about extends, and the awk quote-stripping implementation in check-node-version.sh is unnecessarily fragile/hard to maintain as written.

Review details

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

README.adoc:114

  • The README says extends ensures all services run the same image, but Compose still allows overriding image:; consider wording this as a convention enforced by this repo (and the runtime check) rather than a hard guarantee.
    versions.yml:9
  • The comment claims extends means services cannot define a different image, but Compose still allows an explicit image: override; this wording overstates the guarantee and can mislead maintainers.

versions.yml:23

  • This inline comment is unclear/redundant ("Update the default version below for CONTAINER_IMAGE_TAG..."); it should state the default and how to override it.
    # Update the default version below for CONTAINER_IMAGE_TAG or an environment variable to update this.

scripts/check-node-version.sh:147

  • The gsub(/["'\'']/, "") line relies on a hard-to-read shell quoting trick and is easy to break when editing; using explicit patterns for double-quote and apostrophe avoids embedded single-quotes inside the single-quoted awk program.
    gsub(/["'\'']/, "")
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@chargio

chargio commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

The comments from Copilot will not be enforced:

  • The possibiliy to test a different base image is something good, and is explained in the file. No need for updates.
  • The gsub suggestion is actually the same that is in the code, it is requesting to update something to the same code.

For that reason, everything will remain as it is

@antgamdia antgamdia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Personally, I don't like the overall direction of this PR. While I do firmly believe keeping consisntecy across versions is of paramount importance, the way it is achieved here... I'm not so sure.

In other repos, we just rely on the .tools-version file from an already known and used asdf package manager.

From CI, getting a version is a simple as:

# Read tool versions for environment setup.
- uses: endorama/asdf-parse-tool-versions
  name: Read .tool-versions

# Setup Go environment.
- uses: actions/setup-go
  with:
    go-version: ${{ env.GOLANG_VERSION }}  # an env var was added in the previous step

In other places, we set up Node.js from the standard runner image, plus a setup node action. I think we should follow the same approach for inter-repo consistency (source):

      - name: Setup Node
        id: setup-node
        uses: actions/setup-node
        with:
          node-version: ${{ matrix.nodejs }}

Replacing that in favor of custom script executions each time a CI step is run... I don't think is a good idea.

On the other hand, I do see the value on fetching the latest images from our registry, but I think this process should happen on a separte workflow, so that the CI does not have to know about any check versions logic whatsoever.

If the logic is moved to a GHA, then the changes are more targeted and don't "pollute" the existing workflows. I know the intention was ensuring version consistency or fail otherwise, but I think we don't really need that check that very deep in the CI logic itself and it could be moved to a more "out-of-band" place.

Moreover, the PR itself if mixing concerns: we should not be fixing a typo in another file in a PR modiying our CI: were we going to revert it, we would lose the fix.

The readme adds references to non-used package managers (e.g., mise), which sounds may be misleading for users to read in the very top readme file.

I'd rather:

Anyway, this is just my personal opinion, so let's others chime in. This is also a "non-critical" repo, so we might want to "experiment" here.

@nelsonkopliku

Copy link
Copy Markdown
Member

Thanks @antgamdia for anticipating me.

I also suggest to use endorama/asdf-parse-tool-versions to use .tool-versions versions in CI.

Additionally, if we want to use the same versions in the container we might consider simplifying version extraction to awk '!/^#/ && $1=="nodejs" {print $2; exit}' .tool-versions which returns 24.18.1, export is as an env variable and let docker-compose get it.

The rest of the changes about checking compatibility between what we have in .tool-versions and what we have available in BCI might be very well a separated workflow, as already suggested, if we really feel we need them.

@chargio chargio closed this Sep 21, 2026
@chargio

chargio commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Closing the PR in favor of opening a new one following the direction of the commits.

@chargio
chargio deleted the small_fixes branch September 21, 2026 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation

Development

Successfully merging this pull request may close these issues.

4 participants