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

Using --default option #133

Merged
merged 11 commits into from
Oct 8, 2020
Merged

Conversation

khanalpride
Copy link
Contributor

I have worked out on using --default on any enable command to use default ports and tags or keys

can be useful if you create many services at once as per your project setup
#130

I have edited the Environment.php to check ports availability on Linux and WSL
And I have edited the container name, so that it will not conflict when creating 2 docker container with same version but with different ports

@khanalpride
Copy link
Contributor Author

@mattstauffer all tests pass on local.
And yeah it was difficult to add tests. :(

@josecanhelp
Copy link
Contributor

@khanalpride Thanks so much for this PR. I'm still working through a few adjustments I'd like to make. Mainly, I was hoping to use a -y flag instead of --default, but I can't remember if this was possible with Laravel Zero. If you can figure it out, please add it to the PR. Otherwise, I'll get this merged in after I figure it out.

Great work!

@khanalpride
Copy link
Contributor Author

@josecanhelp sure.
i will check that out.
thanks.

@khanalpride
Copy link
Contributor Author

@josecanhelp I am still trying to find way for -y flag.

May be @nunomaduro can provide some clue regarding -y flag here. So I can implement it.

@khanalpride
Copy link
Contributor Author

@josecanhelp just added --all to disable command
so we can disable all docker container using just
takeout disable --all

@khanalpride
Copy link
Contributor Author

@josecanhelp @mattstauffer
This PR has no response? Is it being considered?
Or it needs more work?

@josecanhelp
Copy link
Contributor

Hey @khanalpride sorry for the delay. I am still considering this PR, but I just haven't had the time to review it. I will update you soon. Thanks again for this work!

Copy link
Contributor

@josecanhelp josecanhelp left a comment

Choose a reason for hiding this comment

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

I had a few requests, but this is really great work! I can't wait to get it merged in and deployed.

Thank you.

app/Commands/DisableCommand.php Outdated Show resolved Hide resolved
app/Commands/DisableCommand.php Outdated Show resolved Hide resolved
app/Commands/EnableCommand.php Show resolved Hide resolved
app/Services/BaseService.php Outdated Show resolved Hide resolved
app/Shell/Environment.php Show resolved Hide resolved
@josecanhelp
Copy link
Contributor

Another request would be to only have 2 dashes between before the port number in the container name:
image

@khanalpride
Copy link
Contributor Author

@josecanhelp I have checked and reviewed your suggestion.
Please check and let me know. :)

Copy link
Contributor

@josecanhelp josecanhelp left a comment

Choose a reason for hiding this comment

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

Again, great work on this! 👏

@josecanhelp josecanhelp merged commit 76912c2 into tighten:main Oct 8, 2020
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

3 participants