-
Notifications
You must be signed in to change notification settings - Fork 0
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
Dev docker wrapper #7
base: master
Are you sure you want to change the base?
Conversation
Ah crap, hold fire. SSH tunnels don't work 😂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job all around @spamoom, will be very useful!
Other than the comments already added, the major problem I have is that the docker compose
command is not available in stable docker releases yet, so while I am up to date on docker, unless I switch to testing/edge, it's not going to work for me.
What do you think of using docker-compose for the time being (keeping the same major NS CLI version), and then doing the switch to just docker
once the feature lands in stable?
Another thing I just thought about. Does the AWS ECR access work properly?
When we run the docker:aws:login
command, that then runs the docker login command, which stores the auth token in ~/.docker/config.json
- is this written to the host or the container? And also, is this file read by the CLI or the daemon? Depending on those, it could work or it wouldn't. I have hard time checking that though due to the lack of docker compose
on my machine.
## | ||
# PHP Runtime | ||
## | ||
FROM php:7.4-cli as runtime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do few tweaks here to make the resulting image considerably smaller, and also to make the self-update pull down only the things that changed. Are you ok for me to look into it later on in the evening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely mate - I was doing this between meetings so it's very WIP - just wanted it in front of you as I knew you'd spot a load of improvements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, will try few ideas and report back :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done few tweaks to reduce the image by > 300MB. It's still big, but at least it has all tools inside it which is convenient.
ECR helper works well after a fix which is great, however worth testing again on TeamCity, as it won't have access to the AWS config file I think?
I do have problems using the SSM connect feature, but not sure if it's just all our EC2 instances not having it enabled or something? Would be nice if you can confirm it's working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work - yeah the NS EC2 images aren't setup for SSM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already checked on the YPS Teamcity agent, it's able to access the meta domain so is able to get auth 🌮
Thinking further about this - I think it might be wise to drop non docker usage all together- thoughts? |
If we manage to get all features to work, I don't see why not. We could then have the whole netsells CLI installer from now.sh just install the wrapper script. However I think I would wait with this until we don't need |
--rm \ | ||
--init \ | ||
-it \ | ||
-v 'sshvol:/root/.ssh' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is still required, given we now store the key in tmp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't sync with the local ssh, it just uses a volume to persist the known_hosts file so you don't have to type yes
every time you connect to the same server
This is the Dockerfile I created for testing:
|
Uh oh! Looks like this PR has some conflicts with the base branch, @spamoom. Please bring your branch up to date as soon as you can. |
Introduces the Dockerfile and a bundled wrapper for running and managing.
Only changes to actual CLI code are removing the need to use .ssh and making use of the local image directory as well as dropping support for
docker-compose
and using the new built indocker compose
Due to the docker compose change, this will be tagged as a major version bump