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

Install, Enable and Disable SSLCertificateCommands #82

Closed
wants to merge 14 commits into from
Closed

Install, Enable and Disable SSLCertificateCommands #82

wants to merge 14 commits into from

Conversation

solocal-ecommerce
Copy link

Missing Install, Enable and Disable SSLCertificateCommands.
Thanks for your work,
Thomas

@typhonius
Copy link
Owner

Thanks for this, I've just committed the enable/disable task in #83

If you could rebase this PR on master and include tests for the install task (as #83 did) then I'll get that merged in. You'll be wanting to look at the SslCertificateCommandTest class for that, as well as adding in a reference to the right fixture in AcquiaCliTestCase.

@typhonius
Copy link
Owner

We should also consider how we manage SSL certificates, keys etc as these may be unwieldy to pass in on the command line. We could potentially pass those in as paths to files too. I'd be keen to solicit information from users to determine what the most appropriate idea would be,

@typhonius
Copy link
Owner

Could of review comments:

  • Looks like there's code already committed for ssl:enable / ssl:disable so you can probably remove that and focus only on ssl:create
  • Is it likely that users will be able to upload certificates on the command line? They can be really really really long so I wonder if we should point to a certificate file to parse. Thoughts?
  • Could you please fix the code sniffer errors below
➜  acquia-cli git:(solocal-ecommerce-master) composer cs
> phpcs --standard=PSR12 -n src tests

FILE: ...admalone/repos/acquia-cli/src/Commands/SslCertificateCommand.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 15 | ERROR | [x] Opening brace of a class must be on the line after
    |       |     the definition
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...ne/repos/acquia-cli/tests/Commands/SslCertificateCommandTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 81 | ERROR | [x] Expected 1 newline at end of file; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 543ms; Memory: 10MB

Script phpcs --standard=PSR12 -n src tests handling the cs event returned with error code 2

- certificates can now be passed as input files
@solocal-ecommerce
Copy link
Author

Hi,

  • code sniffer errors have been fixed
  • That was easier for me to pass certificates as env variables, but you're right, it will be more generic to pass them using file path. I have refactored it and should now be ok.

@typhonius
Copy link
Owner

Thanks for your work on this @solocal-ecommerce - I've made the changes I recommended in #119 and committed it to master. It will be part of the next tagged release which can be downloaded with the self:update command if you are using the phar Cli tool.

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