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

Update Docker files (post-Symfony migration) #169

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

ChlodAlejandro
Copy link
Contributor

This fixes the Docker configuration and Docker-related CI runs following the Symfony migration (#168).

  • Moved Dockerfile to docker/ folder
  • Run post-install scripts later on in the build
  • Move source file copy to end of target build
  • Add OpenSSH, Symfony CLI, and Composer to development image
  • Add entrypoint script (/docker-entrypoint.sh)
    • Add option to start Symfony local development server
    • Add option to start ToolforgeBundle SSH connection from image
  • Optimize Dockerfile
  • Add .dockerignore (copy of .gitignore)
  • Fix docker-compose.dev.yml to conform to new project structure
    • Removed copypatrol-redis service
    • Automatically mount $HOME/.ssh to development container on startup
    • Change mounted folders to fit new structure
  • Point ci.yml to new Dockerfile location
  • Upgrade Docker-related CI steps

@ChlodAlejandro ChlodAlejandro changed the title Update Dockerfile (post-Symfony migration) Update Docker files (post-Symfony migration) Jul 25, 2023
Copy link
Member

@MusikAnimal MusikAnimal left a comment

Choose a reason for hiding this comment

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

I'm assuming we need wikimedia/ToolforgeBundle#61 before this will fully work?

Just in case it's unrelated: Following the directions in the README, everything worked great up until trying to open up the SSH tunnel. For me, it assumed root@login.toolforge.org. I'm not sure if that's expected. My local shell name is the same as on Toolforge, so as per the instructions I tried:

docker compose exec copypatrol start ssh musikanimal

but that gave me Too many arguments to "toolforge:ssh" command, expected arguments "username".

README.md Outdated
symfony console toolforge:ssh --bind-address=0.0.0.0 --toolsdb
# (optional) Prevent double-downloading when the build occurs.
docker image pull docker-registry.tools.wmflabs.org/toolforge-php74-sssd-base:latest
docker-compose build
Copy link
Member

Choose a reason for hiding this comment

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

is the dash intentional? I've seen both, but on my machine I had to use docker compose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems docker compose is the new, suggested command. docker-compose refers to an old version of Docker Compose.[1] This doesn't have much effect on Windows, but I guess that's not the case everywhere. I'll have this fixed in a later patch.

@ChlodAlejandro
Copy link
Contributor Author

ChlodAlejandro commented Jul 25, 2023

Following the directions in the README, everything worked great up until trying to open up the SSH tunnel.

wikimedia/ToolforgeBundle#61 shouldn't be required, but if I file a patch for the following (third paragraph), it then might be. The issue that these lines attempt to solve is that SshCommand doesn't respect the User set in the user's .ssh/config file, and applies the current username regardless of that value (which would be machine\user on Windows systems). Submitting a username makes the command even longer than it currently is.

A user passing in their shell name should have been considered, but it seems that patch was lost, probably while I was (un)stashing around different solutions I had for the Docker fixes. The fix would be just avoiding the user of $username entirely when a variable without a dash is passed.

I could do the aforementioned, but I think it would be better to make ToolforgeBundle respect config-set Users instead, since this would have been expected behavior for someone with the value set in their SSH config. I didn't want to include this as part of wikimedia/ToolforgeBundle#61 though, since it would be a somewhat unexpected (if not breaking) change. Should I file a patch for this in ToolforgeBundle or just handle that behavior with this PR?

@samwilson
Copy link
Member

I've released a new version of ToolforgeBundle with those changes.

I like the idea of respecting the user's ssh config, if that's going to work. I guess there might be situations where someone hasn't configured ssh for Toolforge, but they'd likely be rare.

@ChlodAlejandro
Copy link
Contributor Author

ChlodAlejandro commented Jul 25, 2023

Got it, I'll work on a patch for the above two in a bit. Thank you, @samwilson!

* Moved Dockerfile to `docker/` folder
* Run post-install scripts later on in the build
* Move source file copy to end of target build
* Add OpenSSH, Symfony CLI, and Composer to development image
* Add entrypoint script (`/docker-entrypoint.sh`)
  * Add option to start Symfony local development server
  * Add option to start ToolforgeBundle SSH connection from image
* Optimize Dockerfile
* Add .dockerignore (copy of .gitignore)
* Fix docker-compose.dev.yml to conform to new project structure
  * Removed `copypatrol-redis` service
  * Automatically mount `$HOME/.ssh` to development container on startup
  * Change mounted folders to fit new structure
* Point ci.yml to new Dockerfile location
* Upgrade Docker-related CI steps
@ChlodAlejandro
Copy link
Contributor Author

ChlodAlejandro commented Jul 25, 2023

Fixed the documentation issue and made /app read-write from within the container (so that Composer actions, etc. may be performed within the container environment). SSH issue is pending the result of wikimedia/ToolforgeBundle#63.

Copy link
Member

@MusikAnimal MusikAnimal left a comment

Choose a reason for hiding this comment

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

Works now in my testing! wikimedia/ToolforgeBundle#63 was merged but a new release wasn't cut yet, so I tested with:

docker compose exec copypatrol symfony console toolforge:ssh --toolsdb -b 127.0.0.1 musikanimal

and it all worked flawlessly.

The possible issue with .dockerignore becoming outdated can be addressed in a separate PR if desired.

Thanks so much for contributing!

###> symfony/phpunit-bridge ###
.phpunit.result.cache
/phpunit.xml
###< symfony/phpunit-bridge ###
Copy link
Member

Choose a reason for hiding this comment

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

Will this get updated by Symfony Flex like the normal .gitignore will? I'd just hate to have the two go out of sync. I assume this is to guard against committing things within the container. Would a symlink to the local .gitignore accomplish the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://symfony.com/doc/current/setup/docker.html tells me that it might be, but I'm not so sure. Might be worth testing. But in any case, a symlink should work too.

@MusikAnimal MusikAnimal merged commit d67148e into wikimedia:master Jul 25, 2023
5 checks passed
@ChlodAlejandro
Copy link
Contributor Author

Looks like the change on ToolforgeBundle has been tagged. Best to update the package soon so that the username issue won't occur down the line for other devs. Thank you both, @samwilson and @MusikAnimal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants