-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(gcp): add a test_connection
method
#4616
Conversation
test_connection
method
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4616 +/- ##
==========================================
- Coverage 89.12% 89.02% -0.11%
==========================================
Files 913 913
Lines 27866 27890 +24
==========================================
- Hits 24836 24828 -8
- Misses 3030 3062 +32 ☔ View full report in Codecov by Sentry. |
os.environ["GOOGLE_APPLICATION_CREDENTIALS"] = client_secrets_path | ||
@staticmethod | ||
def test_connection( | ||
credentials_file: str, service_account: str, raise_on_exception: bool = True |
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.
Is it mandatory to set the credentials_file
or service_account
? If not please set some default values.
credentials_file: str, service_account: str, raise_on_exception: bool = True | ||
) -> Connection: | ||
""" | ||
Test the connection with the provided credentials file or service account to impersonate |
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.
Improve the docstring with the possible exceptions raised, what the method do and more about the args and returns please.
logger.critical( | ||
"Cloud Resource Manager API has not been used before or it is disabled. Enable it by visiting https://console.developers.google.com/apis/api/cloudresourcemanager.googleapis.com/ then retry." | ||
) | ||
raise 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.
Remember to add if raise_on_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 add tests for the test_connection
method.
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.
🔝 Thanks for this amazing feature!!
Context
We need to be able to test the connection to each provider using one of the available authentication methods, starting from the environment variables.
Description
Add a
test_connection
method for the GCP provider.Create a
test_connection
abstract method in the Provider class but not enforced for now not to break anything.Checklist
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.