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

Abhi/compose cli #217

Merged
merged 33 commits into from Oct 26, 2022
Merged

Abhi/compose cli #217

merged 33 commits into from Oct 26, 2022

Conversation

pretentious7
Copy link
Contributor

This adds the cli proposed in #199

I'm still working on this (need to add in certs and make the guac-firefox default open up to plane.dev)
but I wanted some comments on the cli, the way it works is:
./plane-dev -[lmxg] where
-l : linux
-m : mac
-x : x11
-g : guac

I'd have nice long options, but can't do that without GNU getopts, which isn't on macs by default, so I'd rather not.
what'd you guys think?

@paulgb
Copy link
Member

paulgb commented Oct 19, 2022

Do we need getopts for this? If there are only four options, can't we just do string comparisons?

I'd rather support only long options than only short options, especially since these are all pretty short.

@pretentious7
Copy link
Contributor Author

well I also wanted to add a flag for compose up vs down, and I think it makes the script more readable, but I suppose it makes the command less readable, so ye I'll just match on the longopts (does remove the option to do -xl tho, which is nice and short)

@paulgb
Copy link
Member

paulgb commented Oct 19, 2022

One thing I'm a bit worried about here is that we break docker compose down, docker compose build, etc functionality by doing this. One solution is to:

  • Have a default docker-compose.yml file that includes the guac mix-in
  • Have the CLI overwrite the docker-compose.yml with the appropriate mix-in
  • Instruct the user to run docker compose up after doing so

In particular, it's important to me that the getting started guide not introduce any additional complexity with the CLI, which is why I like keeping a default compose file with the guac mix-in around. Then we can have another section on local development that talks about x11.

@paulgb
Copy link
Member

paulgb commented Oct 19, 2022

Ah, I was writing my comment before I saw yours, but we have similar concerns. Lmk what you think of having the script just be for creating the compose file, and then continuing to use docker compose commands to go up and down.

I guess one concern here is that there may be commands we want to run to set up the x11 server on mac that have to be run for each invocation after a restart?

@pretentious7
Copy link
Contributor Author

I think I know how to deal with that, looking at xquartz docs, but I'll have to make sure.

@pretentious7
Copy link
Contributor Author

hm yea i think that works perfect, I'll have the firefox service be defaulted to guac in the compose file, and -f firefox-x11.yml to override it in the x11 cases. this keeps docker-compose up / docker-compose down useful.

@pretentious7
Copy link
Contributor Author

pretentious7 commented Oct 19, 2022

In that case, if this is to be slightly off the beaten track, can we keep the current getopts handling of cli args? I'll make the usage function nicer.

EDIT: nevermind this, I just remembered an old trick that'll come in handy.

@pretentious7 pretentious7 force-pushed the abhi/compose-cli branch 2 times, most recently from 2f8f466 to 7ae26c7 Compare October 21, 2022 17:51
@pretentious7 pretentious7 marked this pull request as ready for review October 21, 2022 23:22
@render
Copy link

render bot commented Oct 21, 2022

@pretentious7
Copy link
Contributor Author

ahh I think this is ready to be merged, just needs some testing on macos first. (I'll add in additional docs in a future pr, the current workflow is unaffected aside from CD'ing to a different dir, which I changed in the docs)

could one of you try it on mac and make sure I didn't break anything? try ./cli/plane-dev.sh -mg from the sample-config dir or just docker compose up in the plane/sample-config/compose dir

@paulgb
Copy link
Member

paulgb commented Oct 22, 2022

When I run that, I get:

cli/plane-dev.sh: line 63: xdg-open: command not found

Once it starts, I get this:

Error response from daemon: Invalid address 172.16.238.10: It does not belong to any of this network's subnets

Copy link
Member

@paulgb paulgb left a comment

Choose a reason for hiding this comment

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

I like this as long as we can make it 100% seamless on Mac. I do worry about the support burden though, there's a lot of moving parts and unknowns.

sample-config/x11-firefox/Dockerfile Outdated Show resolved Hide resolved
sample-config/x11-firefox/Dockerfile Outdated Show resolved Hide resolved
sample-config/guac-firefox/policies.json Outdated Show resolved Hide resolved
sample-config/guac-firefox/Dockerfile Outdated Show resolved Hide resolved
sample-config/guac-firefox/Dockerfile Outdated Show resolved Hide resolved
sample-config/guac-firefox/policies.json Outdated Show resolved Hide resolved
sample-config/compose/plane.yml Show resolved Hide resolved
sample-config/compose/plane.yml Outdated Show resolved Hide resolved
@pretentious7
Copy link
Contributor Author

When I run that, I get:

cli/plane-dev.sh: line 63: xdg-open: command not found

Once it starts, I get this:

Error response from daemon: Invalid address 172.16.238.10: It does not belong to any of this network's subnets

Ah can you try running it after a docker compose down? Looks like the invalid address issue there comes from it using the wrong network.

I also totally forgot the freedesktop stuff wasn't POSIX. Will fix.

@pretentious7
Copy link
Contributor Author

I'll add in the x11 support for mac once I have the aws VM set up, I think this should be good to merge for now though.

@paulgb
Copy link
Member

paulgb commented Oct 25, 2022

This still has the issue we discussed where it wants to build the drone locally when I don't have the image already, and we don't have a workaround for that without causing the other images not to build. I'm inclined to just go back to commenting out the build section of the yaml, I think.

Copy link
Member

@paulgb paulgb left a comment

Choose a reason for hiding this comment

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

One small doc change but otherwise looks good!

@@ -37,7 +37,7 @@ Use docker-compose to start a local instance of Plane:

```bash
cd plane/sample-config/compose
docker compose up
docker compose pull && docker compose up
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's just do these on separate lines, i.e.

cd plane/sample-config/compose
docker compose pull
docker compose up

That way it's easier to debug because the output is directly associated with the command that caused it.

Copy link
Member

@paulgb paulgb left a comment

Choose a reason for hiding this comment

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

Oops, thought I LGMT'd this on the last comment. Thanks for the change, merging now!

@paulgb paulgb merged commit edc7b13 into main Oct 26, 2022
@paulgb paulgb deleted the abhi/compose-cli branch October 26, 2022 13:44
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.

None yet

2 participants