Skip to content

Upgrade docker/down.sh to compose v2#2678

Merged
wslulciuc merged 8 commits into
mainfrom
docker/linux-down
Feb 14, 2024
Merged

Upgrade docker/down.sh to compose v2#2678
wslulciuc merged 8 commits into
mainfrom
docker/linux-down

Conversation

@merobi-hub

@merobi-hub merobi-hub commented Nov 5, 2023

Copy link
Copy Markdown
Collaborator

Problem

The down script in docker does not execute properly on at least one widely-used flavor of Linux. It turns out that the issue was actually being caused by the version of Docker Compose, not the OS.

Solution

This migrates the script to Compose v2. This should be safe, as v1 has been deprecated for a while now.

Note: All database schema changes require discussion. Please link the issue for context.

One-line summary: migrates docker/down.sh to compose v2

Checklist

  • You've signed-off your work
  • Your changes are accompanied by tests (if relevant)
  • Your change contains a small diff and is self-contained
  • You've updated any relevant documentation (if relevant)
  • You've included a one-line summary of your change for the CHANGELOG.md (Depending on the change, this may not be necessary).
  • You've versioned your .sql database schema migration according to Flyway's naming convention (if relevant)
  • You've included a header in any source code files (if relevant)

@boring-cyborg boring-cyborg Bot added the docker label Nov 5, 2023
@merobi-hub merobi-hub requested a review from wslulciuc November 5, 2023 13:31
@netlify

netlify Bot commented Nov 5, 2023

Copy link
Copy Markdown

Deploy Preview for peppy-sprite-186812 canceled.

Name Link
🔨 Latest commit d2b1d71
🔍 Latest deploy log https://app.netlify.com/sites/peppy-sprite-186812/deploys/65cd0006f69d7000083d0791

@codecov

codecov Bot commented Nov 5, 2023

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (87dcdf6) 80.63% compared to head (1c338ca) 84.42%.

❗ Current head 1c338ca differs from pull request most recent head d2b1d71. Consider uploading reports for the commit d2b1d71 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2678      +/-   ##
============================================
+ Coverage     80.63%   84.42%   +3.79%     
- Complexity      234     1416    +1182     
============================================
  Files            43      251     +208     
  Lines           976     6434    +5458     
  Branches         38      291     +253     
============================================
+ Hits            787     5432    +4645     
- Misses          160      850     +690     
- Partials         29      152     +123     

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

@merobi-hub merobi-hub changed the title Add option to remove volumes without Compose to down script Add option to docker/down to remove volumes without Docker Compose Nov 5, 2023
@merobi-hub merobi-hub force-pushed the docker/linux-down branch 3 times, most recently from 27dc322 to d8ce15e Compare November 5, 2023 16:30
Signed-off-by: merobi-hub <merobi@gmail.com>

@wslulciuc wslulciuc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since docker/up.sh requires docker compose, I'm not sure we should have an exception for docker/down.sh. My suggestion would be that the we add a check in docker/up.sh ensuring that docker compose is installed, if not, log a warning that only the volumes will be deleted (flags --volumes, --help would no longer be needed).

Signed-off-by: merobi-hub <merobi@gmail.com>
@merobi-hub merobi-hub changed the title Add option to docker/down to remove volumes without Docker Compose Add docker compose cmd to docker/down for Linux Nov 8, 2023
@merobi-hub

merobi-hub commented Nov 8, 2023

Copy link
Copy Markdown
Collaborator Author

@wslulciuc I did a little more experimenting and found that a differently formatted Compose command will work on Linux if you are using the Compose plugin, which is probably common as an alternative to Docker Desktop. So I removed the flag (but kept the help one because, why not? -- we have a usage string) and the command-removal condition. Instead of avoiding the command, this now checks for Linux and executes a differently formatted version of the command. It's been tested on Mint and works as long as the env vars in docker-compose.yml have been set. I would be happy to implement the change you suggested in docker/up and add a condition that checks for Compose and omits the command if not found. Please let me know WYT.

@merobi-hub merobi-hub requested a review from wslulciuc November 8, 2023 12:05
@merobi-hub merobi-hub changed the title Add docker compose cmd to docker/down for Linux Add Linux-friendly docker compose cmd to docker/down Nov 8, 2023
@merobi-hub merobi-hub changed the title Add Linux-friendly docker compose cmd to docker/down Add Linux-friendly Docker Compose cmd to docker/down Nov 8, 2023
Comment thread docker/down.sh Outdated
if [[ "$OSTYPE" == "linux-gnu" ]]; then
docker compose ${compose_files} down ${compose_args}
else
docker-compose $compose_files down $compose_args

@wslulciuc wslulciuc Nov 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can fully migrate off docker compose v1 to v2. That is, we can replace docker-compose with docker compose as docker compose v1 is no longer receiving updates after July 2023 (of this year). This would also simplify the changes in the PR ;)

@merobi-hub merobi-hub changed the title Add Linux-friendly Docker Compose cmd to docker/down Migrate docker/down.sh to compose v2 Dec 18, 2023
@merobi-hub merobi-hub requested a review from wslulciuc December 18, 2023 17:44
@merobi-hub

Copy link
Copy Markdown
Collaborator Author

Tested again today and can confirm it works

@merobi-hub merobi-hub changed the title Migrate docker/down.sh to compose v2 Upgrade docker/down.sh to compose v2 Jan 10, 2024
@wslulciuc wslulciuc enabled auto-merge (squash) February 14, 2024 18:01
@wslulciuc wslulciuc merged commit 32ae5cb into main Feb 14, 2024
@wslulciuc wslulciuc deleted the docker/linux-down branch February 14, 2024 18:12
@dolfinus dolfinus mentioned this pull request Mar 28, 2024
7 tasks
jonathanpmoraes referenced this pull request in nubank/NuMarquez Feb 6, 2025
* Adds no-compose option and arg and help arg to docker/down.

Signed-off-by: merobi-hub <merobi@gmail.com>

* Adds os check and compose command for linux.

Signed-off-by: merobi-hub <merobi@gmail.com>

* Migrate down cmd to compose v2.

Signed-off-by: merobi-hub <merobi@gmail.com>

---------

Signed-off-by: merobi-hub <merobi@gmail.com>
Co-authored-by: Willy Lulciuc <willy@datakin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants