svenluijten / forge-cli

🔥 A command line utility to interact with your Laravel Forge servers, sites, and more.
https://svenluijten.com
MIT License
64 stars 14 forks source link

Add optional wait parameter where needed #24

Closed thoresuenert closed 4 years ago

thoresuenert commented 5 years ago

closes #23

svenluijten commented 5 years ago

Thanks for this! I'll take a look when I get some time (should be this weekend), as well as set up Travis to properly run the (limited) tests we have 🙂

thoresuenert commented 5 years ago

There is a problem with the Certificates:install command, the new sdk added a data payload array. The sdk and the docs are confusing at this point.

What do we want to achieve withinstall:certificate command? from the arguments it looks like we dont want to install a certificate, we want to clone an existing one instead.

From the docs:

HTTP Request

POST /api/v1/servers/{serverId}/sites/{siteId}/certificates/{id}/install

    Payload

{
    "certificate": "certificate content",
    "add_intermediates": false
}
thoresuenert commented 5 years ago

@svenluijten dont waste anytime on this PR.

I had a dive into laravel core how to test the commands etc.

The last commit should be the right way to test and build optional none value flags like wait should be.

Pleas provide feedback which way to go and i will clean this PR.

svenluijten commented 5 years ago

Yeah that last commit is how I would do it as well. The tests still feel quite "fragile", because we're mocking third party dependencies ("don't mock what you don't own"), but writing a wrapper around all those objects feels a bit excessive for now.

All in all, LGTM! Let me know if you need any more pointers 🙂

thoresuenert commented 5 years ago

moved the certificate question into the following issue #27