Skip to content

Conversation

@camalot
Copy link
Contributor

@camalot camalot commented Apr 4, 2023

  • generate a random password for RCON_PASSWORD during the build
  • pass the value to RCON_PASSWORD as a build-arg
  • update the tests to account for the random password
  • update the test runner to pass in environment variables to the execution of verify.sh

Closes #2002

@itzg itzg self-requested a review April 6, 2023 18:29
Comment on lines +15 to 28
setup:
runs-on: ubuntu-20.04
outputs:
# output the generated rcon_password to be used as build arg later.
rcon_password: ${{ steps.passwdgen.outputs.password }}
steps:
# the setup for the rcon password is done outside of the matrix, so each
# build uses the same generated password in all the image variants.
- name: Create RCON Password
uses: licenseware/generate-password-and-hash@v1
id: passwdgen
with:
length: 20
build:
Copy link
Owner

Choose a reason for hiding this comment

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

From a CI perspective, I like the idea of this setup stage. However, I was expecting a simpler approach where the build job just included this step inline, which has the nice side effect of "randomizing" the passwords a bit more, one per image variant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One per image variant is fine too, i just thought to do it once and reuse that, but either way works, since the password doesnt really matter except for the tests.

Comment on lines +49 to 53
# if there is an environment.sh, run it, and push the output to ./.tests.env. then load that as an env-file for the docker run
env_file=$([ -f environment.sh ] && sh ./environment.sh > ./.tests.env && echo "--env-file=./.tests.env" || echo "")
[ ! -z "${env_file// }" ] && echo "Using env-file argument: $env_file"
if ! docker run --rm --entrypoint bash $env_file -v "${PWD}/data":/data -v "${PWD}/verify.sh":/verify "${IMAGE_TO_TEST:-itzg/minecraft-server}" -e /verify; then
endTime=$(date +%s)
Copy link
Owner

Choose a reason for hiding this comment

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

This all seems quite complicated and as previously implemented it was necessary. Looking closer at this I realized I should have been using docker compose run for the verify also. So, I went with an idea there and confirmed it works:

5df0769#diff-f8a6ba079e5e45526e46f87838c9d21b0caff96739fc36d21e3b41ad767f77faR49

If you're good with this, I can just push my changes into your PR branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, so when you use compose run the variables are available? that's great. way less complicated. 👍 yes, lets go with that for sure.

Copy link
Owner

Choose a reason for hiding this comment

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

This file can be removed if going with my proposed changes.

@itzg
Copy link
Owner

itzg commented Apr 6, 2023

...and here's the build testing my proposed changes

https://github.com/itzg/docker-minecraft-server/actions/runs/4632194211/jobs/8195981091

@itzg
Copy link
Owner

itzg commented Apr 8, 2023

Thank you so much for all the effort on this one, but I had an epiphany and realized how I could randomize the default at startup and still allow rcon-cli and mc-backup to work, via #2071 and itzg/docker-mc-backup#134

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.

RCon enabled by default with default password

2 participants