chore: improve build workflows#3262
Conversation
`Command` that allows removing a service from the codebase.
stobrien89
left a comment
stobrien89
left a comment
There was a problem hiding this comment.
A couple of changes requested- Not sure how feasible this is for every class, but this is a lot of new logic with no unit tests. The RemoveServiceCommand class should definitely have some unit tests
build/Command/AbstractCommand.php
Outdated
| } | ||
| } | ||
|
|
||
| protected function parseOptions(array $shortOptions, array $longOptions = []): array |
There was a problem hiding this comment.
getOpt() is notoriously fragile. We should parse the args manually like in RemoveServiceCommand
build/Command/GhReleaseCommand.php
Outdated
| $isSuccessful = true; | ||
| } catch (\GuzzleHttp\Exception\ServerException $e) { | ||
| $this->output("{$filename} failed to upload:"); | ||
| var_dump($e->getMessage()); |
There was a problem hiding this comment.
seems like $this->error() or $this->output() should be called here
build/Command/GhReleaseCommand.php
Outdated
| $this->output("Failed upload of {$asset['name']} at {$asset['browser_download_url']} has successfully been deleted."); | ||
| } else { | ||
| $this->output("Failed upload of {$asset['name']} at {$asset['browser_download_url']} was unable to be deleted."); | ||
| var_dump($e); |
There was a problem hiding this comment.
seems like $this->error() or $this->output() should be called here too
build/Command/OptionDocsCommand.php
Outdated
| } | ||
|
|
||
| $docs = '* - ' . $name . ': ' . $docs; | ||
| echo wordwrap($docs, 70, "\n* ") . "\n"; |
There was a problem hiding this comment.
should $this->output() be called here?
build/Command/GhReleaseCommand.php
Outdated
| function ($retries, $request, $response) { | ||
| $statusCode = $response->getStatusCode(); | ||
| if ($retries < self::MAX_ATTEMPTS && !in_array($statusCode, [200, 201, 202])) { | ||
| echo "Attempt failed with status code {$statusCode}: " |
There was a problem hiding this comment.
should $this->output() be called here too?
| exec('git add ' . $escapedPaths . ' 2>&1', $output, $exitCode); | ||
| if ($exitCode !== 0) { | ||
| $this->error('git add failed: ' . implode("\n", $output)); | ||
| exit($exitCode); |
There was a problem hiding this comment.
Seeing exit() called in a handful of places where maybe an exception should be thrown or an error code returned to doExecute()?
|
|
||
| $changelogBuilder->buildChangelog(); | ||
|
|
||
| // Omit fixEndpointFile() call - method doesn't exist on ChangelogBuilder |
There was a problem hiding this comment.
is this comment intentional?
There was a problem hiding this comment.
Yes, I just want to make sure it keeps documented for future revisions since that method is called here, in the old script file, but is not a member of ChangelogBuilder.
| ); | ||
|
|
||
| $burgomaster->startSection('test-phar'); | ||
| $burgomaster->exec('php ' . $buildDir . '/WorkflowCommandRunner.php test-phar'); |
There was a problem hiding this comment.
could the runner path be moved to a constant?
| return 'php build/WorkflowCommandRunner.php phar-test-runner [-- phpunit/behat options]'; | ||
| } | ||
|
|
||
| protected function doExecute(array $args): int |
There was a problem hiding this comment.
does this actually execute the tests?
yenfryherrerafeliz
left a comment
yenfryherrerafeliz
left a comment
There was a problem hiding this comment.
Not sure how feasible this is for every class, but this is a lot of new logic with no unit tests. The RemoveServiceCommand class should definitely have some unit tests.
Yes, maybe we should have test coverage, however, the reason why I initially decided not to do it was because those scripts did not have either before this improvements, and we are not changing or introducing functionalities, except for the service removal, but just migrating to a different structural and functional architecture. I can definitely add test coverage if needed.
2 participants
Description of changes:
Improve build workflows so they are now executed from a centralized orchestration by following a command-runner architecture.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.