-
Notifications
You must be signed in to change notification settings - Fork 21
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
Domo Integration #350
Domo Integration #350
Conversation
9a5b10c
to
8fd9619
Compare
a0f7189
to
019e45b
Compare
b972913
to
8788557
Compare
336267d
to
e3e7c26
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few changes.
src/Helper/Log/GoogleApiClient.php
Outdated
public function __construct(HttpClientInterface $http_client, | ||
SymfonyStyle $output, | ||
DrupalCoreVersionResolver $coreVersionResolver, | ||
$google_api_client_id, | ||
$google_api_client_secret, | ||
$google_refresh_token) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string arguments can be type-hinted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please re-check this one and let me know what do you think.
// Assigning to null as beta or later version does not exist. | ||
$this->nextMajorLatestMinorBetaOrLater = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for? The method name indicates that it's an assertion, which probably shouldn't have any side effects, and if this line runs it will always be immediately followed by an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please suggest a function name instead of assertNextMajorLatestMinorBetaOrLaterExists
:)
Co-authored-by: Travis Carden <travis.carden@gmail.com>
Co-authored-by: Travis Carden <travis.carden@gmail.com>
Co-authored-by: Travis Carden <travis.carden@gmail.com>
Co-authored-by: Travis Carden <travis.carden@gmail.com>
Co-authored-by: Travis Carden <travis.carden@gmail.com>
* Initial Working implementation * use event dispatcher compatible with php 7.4 * Dont send data when test is skipped * Add env variables to github action workflow * Resolve issues * Fix lock files * Fix all issues * Update src/Console/Command/Ci/CiRunCommand.php Co-authored-by: Travis Carden <travis.carden@gmail.com> * Update src/Enum/EnvVarEnum.php Co-authored-by: Travis Carden <travis.carden@gmail.com> * Update src/Helper/Log/GoogleApiClient.php Co-authored-by: Travis Carden <travis.carden@gmail.com> * Update src/Helper/Log/GoogleApiClient.php Co-authored-by: Travis Carden <travis.carden@gmail.com> * Update src/Helper/Log/GoogleApiClient.php Co-authored-by: Travis Carden <travis.carden@gmail.com> * PR feedback * Changes related to Exit Early Save --------- Co-authored-by: Travis Carden <travis.carden@gmail.com>
@secretsayan @TravisCarden it looks like the ORCA workflow example integration was never updated to add these new secrets. Was that intentional? https://github.com/acquia/orca/blob/develop/example/.github/workflows/orca.yml |
@danepowell Thanks for pointing this out. PR #425 created to modify example files. |
No description provided.