Skip to content

dockerfile improvements#1916

Open
jdfranel wants to merge 12 commits intoNatLibFi:mainfrom
jdfranel:feat/dockerfile-improvement
Open

dockerfile improvements#1916
jdfranel wants to merge 12 commits intoNatLibFi:mainfrom
jdfranel:feat/dockerfile-improvement

Conversation

@jdfranel
Copy link

Reasons for creating this PR

  • The Docker image contains many files that are not needed to run Skosmos in production (mostly development files). This takes unnecessary space and might result in secret leakage.
  • Node 18 reached end of life on April 30, 2025, creating a security risk.

Link to relevant issue(s), if any

N/A

Description of the changes in this PR

This PR improves the Docker build process by reducing the production image size and updating outdated dependencies.

Key changes:

  1. Updated .dockerignore to exclude development and unnecessary files from the Docker build context
  2. Modified Dockerfile to use selective file copying instead of copying the entire workspace
  3. Added a development stage in Dockerfile.ubuntu that includes files needed for testing (used by tests/docker-compose.yml)
  4. Upgraded Node.js from version 18 to 24 in both the Docker image and GitHub workflows

Files excluded from production image:

  • Development configuration: .codeclimate.yml, .jshintrc, phpunit.xml, .sonarcloud.properties
  • Documentation: README.md, CONTRIBUTING.md, LICENSE, SECURITY.md, PULL_REQUEST_TEMPLATE.md
  • Development tools: ansible/, tests/, .github/, dockerfiles/, composer.lock, composer.phar, phpdoc.sh
  • Configuration files: docker-compose.yml, Vagrantfile, cypress.config.js, .gitignore, .dockerignore, ansible.cfg

These changes reduce the attack surface and image size while maintaining full functionality in production.

Known problems or uncertainties in this PR

Removed the git package from the Docker image since there is no .git directory in the production build. This results in a sh: 1: git: not found error appearing in logs on page requests. Options to resolve:

  • Re-add the git package to the image, or
  • Handle the error gracefully to prevent it from appearing in logs

Input needed on the preferred approach.

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@jdfranel jdfranel marked this pull request as ready for review January 29, 2026 12:25
@osma
Copy link
Member

osma commented Jan 29, 2026

Thanks for the PR. There are a lot of good improvements here.

Regarding the git issue: I think this happens because some of the Controller code currently relies on git commands to determine if the code has changed:

protected function getGitModifiedDate()
{
$commitDate = null;
$cache = $this->model->getConfig()->getCache();
$cacheKey = "git:modified_date";
$gitCommand = 'git log -1 --date=iso --pretty=format:%cd';
if ($cache->isAvailable()) {
$commitDate = $cache->fetch($cacheKey);
if (!$commitDate) {
$commitDate = $this->executeGitModifiedDateCommand($gitCommand);
if ($commitDate) {
$cache->store($cacheKey, $commitDate, static::GIT_MODIFIED_CONFIG_TTL);
}
}
} else {
$commitDate = $this->executeGitModifiedDateCommand($gitCommand);
}
return $commitDate;
}
/**
* Execute the git command and return a parsed date time, or null if the command failed.
*
* @param string $gitCommand git command line that returns a formatted date time
* @return DateTime|null
*/
protected function executeGitModifiedDateCommand($gitCommand)
{
$commitDate = null;
$commandOutput = @exec($gitCommand);
if ($commandOutput) {
$commitDate = new \DateTime(trim($commandOutput));
$commitDate->setTimezone(new \DateTimeZone('UTC'));
}
return $commitDate;
}

As long as this is the case, the git command and the .git tree are needed - or at least you will see error messages if you remove them.

IIRC, this check for the latest git commit is a mechanism to invalidate caches when the code changes for example via git pull. It is not very useful within Docker containers though, because the code will presumably never change within a container.

@jdfranel
Copy link
Author

That's what I thought. I guess that the proper way to handle this issue is to prevent the sh: 1: git: not found to appear in the logs.

I also noticed it happens in the phpunit tests but I would argue that it might be better to add git package to the development stage so the tests passes whether it is in a docker container or not.

If it is OK for you I will update this PR so it is properly handled.

@osma
Copy link
Member

osma commented Jan 29, 2026

Sounds good, please do make the changes in this PR!

.dockerignore Outdated
.DS_Store
.phpunit.result.cache
.git
ansible
Copy link
Member

Choose a reason for hiding this comment

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

FYI, the ansible directory was just removed in #1920 so this line is no longer necessary (although it does no harm)

Copy link
Author

Choose a reason for hiding this comment

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

I did removed the ansible directory from the .dockerignore
Is the ansible.cfg also going to be removed ?

Copy link
Member

Choose a reason for hiding this comment

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

Good point. I removed ansible.cfg, along with a few other files, in PR #1923.
The project has a long history and some cruft has accumulated that wasn't actually being used. Thank you for pointing these out!

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 4, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
4 Security Hotspots

See analysis details on SonarQube Cloud

@jdfranel
Copy link
Author

jdfranel commented Feb 6, 2026

git shell calls have been changed to redirect the stderr to /dev/null. I also had to adapt some tests to assert that the missing git is returning null is null so phpunit does not raise a risky test that doe not assert anything. This way, if git is installed on the host, the test swill run as it was previously and if the tests runs on the docker image where there is no git it will pass also.

There is still some SonarCloud Code Analysis that raises an error when I copy all sources recursively in the composer stage. This can be marked as safe since the final stage copy specific files only.

@osma
Copy link
Member

osma commented Feb 6, 2026

Sorry this didn't quite make it into the 3.1 release. There is quite a lot to test and digest here. But we will get back to it soon.

@jdfranel
Copy link
Author

jdfranel commented Feb 6, 2026

Great, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants