search

acquia / cli

Acquia CLI
GNU General Public License v2.0
42 stars 47 forks source link

CLI-1103: Fix #1558: Notification arg formats #1567

Closed danepowell closed 1 year ago

danepowell commented 1 year ago

Motivation

Fixes #1558

Proposed changes

Anywhere that a notification UUID is required as argument, support passing a notification object or URL as well. This basically generalizes the getNotificationUuid logic in app:task-wait to all commands.

Testing steps

  1. Follow the contribution guide to set up your development environment or download a pre-built acli.phar for this PR.
  2. Clear the kernel cache to pick up new and changed commands: ./bin/acli ckc
  3. Try acli app:task-wait with various forms of arguments to ensure they still work:
# Notifications object response
wacli app:task-wait "$(acli api:environments:domain-clear-caches pipelinesvalidation2.dev pipelinesvalidation2dev.prod.acquia-sites.com)"
# Notification UUID
wacli app:task-wait 227eccb1-3345-4a2d-b042-eddd56a78ed7
# Notification URL
wacli app:task-wait https:\/\/cloud.acquia.com\/api\/notifications\/64df4412-20fa-4489-8cbf-1e73095d03eb

@cdubz could you please help test and let me know if it satisfies your request? If you follow the steps above, you can find a pre-built version of CLI for this PR to test with, or you can build it yourself.

cdubz commented 1 year ago

@danepowell thanks for picking this up! This does exactly what I'm looking for and will eliminate a lot of complexity in my CI processes (:

I think there is just one small issue to address -- if I give task-wait an input that is not a valid UUID or notification URL there will be a PHP fatal error in most cases because the code will get all the way to the URL check where it checks for $urlParts[5] without ensuring that array index exists. So we get:

PHP Warning:  Undefined array key 5 in phar:///src/Command/CommandBase.php on line 1068
PHP Fatal error:  Uncaught TypeError: Acquia\Cli\Command\CommandBase::validateUuid(): Argument #1 ($uuid) must be of type string, null given, called in phar:///src/Command/CommandBase.php on line 1069 and defined in phar:///src/Command/CommandBase.php:726

If that can be avoided it will just end up at the generic "Notification format is not one of UUID, JSON response, or URL" which seems fine.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 :tada:

Comparison is base (17d93cd) 91.75% compared to head (db131f9) 91.77%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1567 +/- ## ============================================ + Coverage 91.75% 91.77% +0.02% - Complexity 1657 1663 +6 ============================================ Files 110 110 Lines 5942 5957 +15 ============================================ + Hits 5452 5467 +15 Misses 490 490 ``` | [Impacted Files](https://app.codecov.io/gh/acquia/cli/pull/1567?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=acquia) | Coverage Δ | | |---|---|---| | [src/Command/App/TaskWaitCommand.php](https://app.codecov.io/gh/acquia/cli/pull/1567?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=acquia#diff-c3JjL0NvbW1hbmQvQXBwL1Rhc2tXYWl0Q29tbWFuZC5waHA=) | `100.00% <100.00%> (ø)` | | | [src/Command/CommandBase.php](https://app.codecov.io/gh/acquia/cli/pull/1567?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=acquia#diff-c3JjL0NvbW1hbmQvQ29tbWFuZEJhc2UucGhw) | `94.32% <100.00%> (+0.18%)` | :arrow_up: | | [src/Command/Env/EnvCertCreateCommand.php](https://app.codecov.io/gh/acquia/cli/pull/1567?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=acquia#diff-c3JjL0NvbW1hbmQvRW52L0VudkNlcnRDcmVhdGVDb21tYW5kLnBocA==) | `100.00% <100.00%> (ø)` | | | [src/Command/Env/EnvCreateCommand.php](https://app.codecov.io/gh/acquia/cli/pull/1567?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=acquia#diff-c3JjL0NvbW1hbmQvRW52L0VudkNyZWF0ZUNvbW1hbmQucGhw) | `100.00% <100.00%> (ø)` | | | [src/Command/Env/EnvMirrorCommand.php](https://app.codecov.io/gh/acquia/cli/pull/1567?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=acquia#diff-c3JjL0NvbW1hbmQvRW52L0Vudk1pcnJvckNvbW1hbmQucGhw) | `97.87% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

danepowell commented 1 year ago

Thanks for the feedback! I fixed that and a few other bugs. Feel free to hammer on the new version, otherwise I'll merge this after getting coverage up.