Use Prompts #328
Use Prompts #328
Conversation
gcavanunez
changed the title
gcavanunez
changed the title
|
After upgrading to This upgrade does require |
|
@gcavanunez I'm super interested in getting this merged. The install box has gotten out of hand! Do you need any help to get this moved forward? |
|
@mattstauffer 100%!! Since this guy depends on bumping up to the latest version of laravel-zero, taking a look at this PR could trickle this one out |
gcavanunez
changed the title
Co-authored-by: Tony Messias <[email protected]>
Co-authored-by: Tony Messias <[email protected]>
Co-authored-by: Tony Messias <[email protected]>
Co-authored-by: Guillermo Cava <[email protected]>
tonysm
changed the title
…host instead of localhost This works on Windows and Mac, but will require Linux users to add `--add-host=docker.internal.host:host-gateway` to their aliases. The docs was updated.
|
|
||
| foreach ($questions as $prompt) { | ||
| $items[] = match (true) { | ||
| Str::contains($prompt['shortname'], 'port') => $this->askQuestion($prompt, $this->useDefaults, validate: function (string $port) { |
There was a problem hiding this comment.
Why are we moving to Str::contains? It makes it less clear what we're doing.
There was a problem hiding this comment.
This was duplicated. For the default prompts we were checking shortname === port but for the custom prompts (the services might have other port prompts that are called something_port, for instance), we were checking Str::contains(shortname, 'port'), so I figured it would be better to merge the prompts and just check once with Str::contains, which handles both cases.
app/Shell/Environment.php
Outdated
| // To check if the socket is available, we'll attempt to open a socket on the port. | ||
| // If we cannot open the socket, it means there's nothing running on it, so the | ||
| // port is available. If we are successful, that means it is already in use. | ||
|
|
There was a problem hiding this comment.
Drop the line after the comment block.
Also, I like that you made the fun Laravel -style three comments lines.. but unfortunately, I think we need a bit more -- or a bit different -- explanation. What's missing here is context for anyone who doesn't understand sockets. The natural expectation is that if I try to do a thing on a port and it works, that means the port was available, and if it fails, that means the port wasn't available. But this is the opposite. I think a good comment would explain that, more than explaining exactly what the following few lines do.
|
@mattstauffer ready for another review |
Changed
nunomaduro/laravel-console-menudependency together with theext-posixandext-pcntlextensions requirement (posixwas only used by the menu, not sure why we hadpcntllisted since we're not using process signals)netstatbeing installed. Instead, we will switch to attempting to do afsockopen()on the port. If it returns a resource, it means something is running on that port, so it's NOT available. Otherwise, it returns false, which means nothing is running on it so the port is available. We're using theTAKEOUT_CONTAINER=1environment variable (already present in Takeout's Docker image) to detect if we should checklocalhostorhost.docker.internalhost instead. We're also updating the docs to add the--add-hostoption needed for this to work on Linux (it already works without it on Windows and Mac).logscommand now accepts the name of the service... and when passed nothing, it will prompt the user with the options of running Takeout containerscloses #324
closes #282