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

Improve down.sh script #2778

Merged
merged 2 commits into from
Mar 31, 2024
Merged

Conversation

dolfinus
Copy link
Contributor

@dolfinus dolfinus commented Mar 28, 2024

Problem

👋 Thanks for opening a pull request! Please include a brief summary of the problem your change is trying to solve, or bug fix. If your change fixes a bug or you'd like to provide context on why you're making the change, please link the issue as follows:

./down.sh script is failing with error message:

WARN[0000] The "API_PORT" variable is not set. Defaulting to a blank string. 
WARN[0000] The "API_ADMIN_PORT" variable is not set. Defaulting to a blank string. 
WARN[0000] The "API_PORT" variable is not set. Defaulting to a blank string. 
WARN[0000] The "API_PORT" variable is not set. Defaulting to a blank string. 
WARN[0000] The "API_ADMIN_PORT" variable is not set. Defaulting to a blank string. 
WARN[0000] The "API_ADMIN_PORT" variable is not set. Defaulting to a blank string. 
WARN[0000] The "TAG" variable is not set. Defaulting to a blank string. 
1 error(s) decoding:

* 'services[api].ports[0]' expected a map, got 'string'

This is because docker-compose.yml contains environment variables substitution, and these variables are not set, unlike up.sh. The issue was introduced in #2678.

After filling up these variables I've got another error:

Error response from daemon: get marquez_data: no such volume

This is because volumes are removed using docker volume rm & docker volume rm chain, and if some volume is already removed, command will exit with error, and other volumes will not be deleted.

Solution

Please describe your change as it relates to the problem, or bug fix, as well as any dependencies. If your change requires a database schema migration, please describe the schema modification(s) and whether it's a backwards-incompatible or backwards-compatible change.

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

  • Fix passing variables to down.sh script
  • Instead of manual volumes deletion rely on docker-compose down -v flag which should do this automatically
  • Make volume deletion optional. Calling dowh.sh without arguments now left volumes intact, users have to pass dowh.sh -v flag to remove them.

One-line summary:

Fixed down.sh script error, added new -v option.

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)

Signed-off-by: Martynov Maxim <[email protected]>
@boring-cyborg boring-cyborg bot added the docker label Mar 28, 2024
Copy link

boring-cyborg bot commented Mar 28, 2024

Thanks for opening your first pull request in the Marquez project! Please check out our contributing guidelines (https://github.com/MarquezProject/marquez/blob/main/CONTRIBUTING.md).

Copy link

netlify bot commented Mar 28, 2024

Deploy Preview for peppy-sprite-186812 canceled.

Name Link
🔨 Latest commit 26845e8
🔍 Latest deploy log https://app.netlify.com/sites/peppy-sprite-186812/deploys/6609d4bd0390a50008e3b7ed

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.47%. Comparing base (e0a85f1) to head (58234de).

❗ Current head 58234de differs from pull request most recent head 26845e8. Consider uploading reports for the commit 26845e8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2778   +/-   ##
=========================================
  Coverage     84.47%   84.47%           
  Complexity     1429     1429           
=========================================
  Files           251      251           
  Lines          6460     6460           
  Branches        299      299           
=========================================
  Hits           5457     5457           
  Misses          850      850           
  Partials        153      153           

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

Copy link
Member

@wslulciuc wslulciuc left a comment

Choose a reason for hiding this comment

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

Thanks @dolfinus for the usage updates 💯 💯 🥇

@wslulciuc wslulciuc enabled auto-merge (squash) March 31, 2024 21:25
@wslulciuc wslulciuc merged commit 0d02cda into MarquezProject:main Mar 31, 2024
15 checks passed
Copy link

boring-cyborg bot commented Mar 31, 2024

Great job! Congrats on your first merged pull request in the Marquez project!

@dolfinus dolfinus deleted the fix-docker-down branch March 31, 2024 21:37
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