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

Allow user-defined container name #58

Merged
merged 11 commits into from
Apr 8, 2024
Merged

Conversation

alexdlukens
Copy link
Contributor

@alexdlukens alexdlukens commented Mar 12, 2024

I ran into #24 when using this action inside a matrix. The workflow was attempting to start multiple mongodb containers, causing all but one to error out.

My solution to this was to allow the container name to be input as a configuration option, and using the matrix to differentiate the container names and ports.

Example:

strategy:
  fail-fast: false
  matrix:
    python-version: ["3.10", "3.11", "3.12"]

steps:
  - name: Calculate port
    id: calc
    run: |
      PYTHON_VER_INT=$(echo "${{ matrix.python-version }}" | tr -d '.')
      PORT=$((27000 + PYTHON_VER_INT))
      echo "port=$PORT" >> $GITHUB_OUTPUT

  - name: Setup MongoDB service
    uses: alexdlukens/mongodb-github-action@fa13afeb0a920f19491292b6819a6129c09fb42b
    with:
      mongodb-version: 4.4
      mongodb-port: ${{ steps.calc.outputs.port }}
      container-name: mongodb-${{ matrix.python-version }}

Edit:

Would like to note that this issue occurred when running the workflow locally via act

@marcuspoehls
Copy link
Member

@alexdlukens Nice addition! Thank you for your contribution! I appreciate your effort. I'm busy today but will review and sort the merge request tomorrow.

@danyalaytekin
Copy link

bump 😬 we would also like to use this fix for act. Thanks both.

@marcuspoehls
Copy link
Member

@alexdlukens Hey Alex, do you know why the tests are failing? Maybe we can get the test pipeline to succeed and then merge the changes 🙂

@alexdlukens
Copy link
Contributor Author

@marcuspoehls I'm not sure why the tests are failing. I will investigate today

@alexdlukens
Copy link
Contributor Author

@marcuspoehls Looks like a test failed because MongoDB did not initialize in 20 seconds. I didn't update anything that would change this behavior, but I could increase the startup timeout on line 59 in https://github.com/supercharge/mongodb-github-action/blob/main/start-mongodb.sh. Please let me know if this solution is acceptable

image
image

Cleanup existing docker container beforehand, if it exists. This only occurs when running GitHub actions in an offline environment (e.g. using act), as there cannot be container conflicts using the GitHub-provided runners.
@marcuspoehls marcuspoehls merged commit 5154f2e into supercharge:main Apr 8, 2024
30 of 36 checks passed
@marcuspoehls
Copy link
Member

@alexdlukens I found the issue: the replica set didn’t start on a custom port. MongoDB always started on the default port. Here’s the fixing commit 7ff7ec9

@danyalaytekin
Copy link

danyalaytekin commented Apr 8, 2024

Thanks so much @alexdlukens and @marcuspoehls. Our job passes locally in act now solely by referencing supercharge/mongodb-github-action@67dfa2e. Which almost seemed too easy but I've reconfirmed it did fail with 1.10.0.

@alexdlukens
Copy link
Contributor Author

alexdlukens commented Apr 8, 2024

@marcuspoehls I am bit confused at why the change in 7ff7ec9 fixed this, as --port doesn't exist on the docker run CLI reference. Is this flag documented somewhere else? Either way, thank you for your help with getting the tests to pass!

Final edit here: I see why this works now. I did not know that args after the image name get passed to the process inside the container. Thanks again!

@marcuspoehls
Copy link
Member

@alexdlukens Hey Alex, you’re right: all arguments after the image name are arguments for the process inside the Docker container. Basically pass-through arguments.

@danyalaytekin Hey Danyal, thank you for confirming that the container name feature works 🙂 That’s nice!

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.

3 participants