Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Top-level docker-compose.yml #1512

Merged
merged 2 commits into from
Oct 31, 2023
Merged

Top-level docker-compose.yml #1512

merged 2 commits into from
Oct 31, 2023

Conversation

namedgraph
Copy link
Contributor

Reasons for creating this PR

docker-compose setup unsuitable for multiple Skosmos deployments

Link to relevant issue(s), if any

Description of the changes in this PR

  • moved docker-compose.yml to the top-level dir
  • fixed relative paths accordingly
  • removed container_name properties
  • replaced host port numbers with env variables with default values

Known problems or uncertainties in this PR

Not tested directly :) But these changes work on our private fork and I was able to deploy 2 Skosmos instances on different ports on the same machine.

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)

* moved `docker-compose.yml` to the top-level dir
* fixed relative paths accordingly
* removed `container_name` properties
* replaced host port numbers with env variables with default values
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (99448fc) 70.08% compared to head (0c0c439) 70.08%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #1512   +/-   ##
=========================================
  Coverage     70.08%   70.08%           
- Complexity     1658     1659    +1     
=========================================
  Files            32       32           
  Lines          4272     4272           
=========================================
  Hits           2994     2994           
  Misses         1278     1278           

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@osma osma added this to the 3.0 milestone Sep 18, 2023
@osma osma self-assigned this Sep 19, 2023
Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

The changes here look good and the reasons behind the changes were already discussed in issue #1511.

The documentation under dockerfiles/README.md needs to be updated as well. Currently it says that the docker compose command should be run under dockerfiles/ which obviously isn't true anymore after these changes. Also, the environment variables for setting the ports should be documented there.

When that is done (@namedgraph can you do it?) then this can be merged to master, i.e. Skosmos 2.x.

After that the changes should be also ported to the skosmos-3 branch because we want to keep the Docker configuration similar for both versions. On the skosmos-3 some additional adjustments to the phpunit and cypress test suite setup need to be done as well, probably just fixing up paths in tests/init_containers.sh that currently assume that the docker-compose.yml file is under dockerfiles/. I can do that once we have this in master.

@namedgraph
Copy link
Contributor Author

I'll try this week.

@osma
Copy link
Member

osma commented Oct 31, 2023

What's the status @namedgraph ? Can you fix dockerfiles/README.md so it syncs with the changes in this PR? Then we could merge this.

Aligned with the new `docker-compose.yml` location
@sonarcloud
Copy link

sonarcloud bot commented Oct 31, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@namedgraph
Copy link
Contributor Author

namedgraph commented Oct 31, 2023

@osma made changes. Are they good enough?

@osma
Copy link
Member

osma commented Oct 31, 2023

Looks good now so I'm merging this. Thanks a lot!

The work still needs to be ported to Skosmos 3.x but I'll take care of that later.

@osma osma merged commit e7e57c8 into NatLibFi:master Oct 31, 2023
7 of 9 checks passed
@osma osma modified the milestones: 3.0, 2.17 Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done (verified in test.dev.finto.fi, set Milestone 3.0 for both issue & PR)
Development

Successfully merging this pull request may close these issues.

docker-compose setup unsuitable for multiple Skosmos deployments
2 participants