-
Notifications
You must be signed in to change notification settings - Fork 93
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
Use Google Auth and Cloud Python APIs instead of gcloud
CLI
#2083
Conversation
Signed-off-by: swastik959 <[email protected]>
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.
You need to add the required GCP libraries to pyproject.toml
first.
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 @swastik959 for the contribution! We really appreciate it. This is definitely a good start, as @fangchenli mentioned, we will need to add the required google python library as a dependency in the pyproject.toml
.
@iameskild I wanted to ask should I modify the check credential method to point to credential json file for explicit parameter input |
@swastik959 that's a good question. I think we can start by supporting OAuth2 authentication for service accounts, this might be helpful: https://googleapis.github.io/google-api-python-client/docs/oauth.html#credentials |
Signed-off-by: swastik959 <[email protected]>
@iameskild hi I have made few commits can you please review them . also can you please tell me how can I test them locally |
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.
Amazing work @swastik959! I will have a closer look later today but in the meantime, do you mind making sure the tests/CI are passing?
I'm picking up this PR to get it across the finish line. It has not seen any activity in almost a year but @swastik959 if you're still interested in working on it, feel free to leave a message here and I can point you in the right direction. |
…ment variable in provider functions
…supposed to be a file
gcloud
CLI
This PR is ready for review. Companion docs PR: nebari-dev/nebari-docs#500 How to test this PR
export GOOGLE_CREDENTIALS="path/to/JSON/file/with/credentials"
export PROJECT_ID="Project ID"
nebari validate -c nebari-config.yaml
nebari render -c nebari-config.yaml
I tested all of this, making sure things were working correctly. Furthermore, I triggered our GCP deployment workflow using a branch with the same contents of this PR. Notes
|
"google-cloud-container==2.49.0", | ||
"google-cloud-iam==2.15.1", | ||
"google-cloud-storage==2.18.0", | ||
"grpc-google-iam-v1==0.13.1", |
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.
I needed to add this library explicitly as our conda tests where failing because of it.
@@ -339,7 +339,6 @@ class GoogleCloudPlatformProvider(schema.Base): | |||
@model_validator(mode="before") | |||
@classmethod | |||
def _check_input(cls, data: Any) -> Any: | |||
google_cloud.check_credentials() |
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.
This is not needed anymore as the regions()
function below will call it under the hood.
Just as a reminder, once this is merged, we will need to remember to update the conda-forge package recipe to reflect the dependencies changes. Its passing our conda test, so we should be okay when doing the release. |
@viniciusdc just a friendly reminder to take a look at this PR when you get a chance. Thanks! |
|
||
|
||
@functools.lru_cache() | ||
def instances(project: str) -> Dict[str, str]: |
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.
This function would be useful to do the same thing as AWS and Digital Ocean do and check instance types are valid before deploying. I'd be okay if you just open an issue to re-add that functionality at some point though. Search the repo for instances(
and see how its used in infrastructure's __init__.py file.
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.
@Adam-D-Lewis great catch. I just created an issue to track that and make sure we add that functionality for Google Cloud.
@marcelovilla Looks like the local integration test failed too. Is there an issue already open for that flaky test. I couldn't find one by searching the error message. |
Uhm, this is interesting. #2641 should've addressed this particular issue. |
@Adam-D-Lewis tests seem indeed to be flaky again. I've reopened #2641 to track that as it should have anything to do with this PR. I re-ran the tests and they're all passing now. |
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.
LGTM!
Thanks for the review @Adam-D-Lewis ! Can you take a look at this companion docs PR when you get a chance? It's a fairly small PR. |
fixes #2019
What does this implement/fix?
Put a
x
in the boxes that applyTesting
Any other comments?