Skip to content
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]: Adapt GCPCluster to be able to initialize the cluster by passing credentials without obtaining from the environment #432

Conversation

RubenBBlazquez
Copy link
Contributor

No description provided.

@RubenBBlazquez RubenBBlazquez force-pushed the feature/adapt-gcp-cluster-to-add-the-posibility-to-pass-gcp-auth-json branch from 64dfc3c to 093042e Compare August 28, 2024 09:41
Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to raise this. It looks very similar to #430, but there are some differences.

I would ask the same questions here:

  • Do we need to bump the minimum version of the google auth to support this?
  • Can you verify tests are passing locally for you?

@RubenBBlazquez
Copy link
Contributor Author

RubenBBlazquez commented Aug 28, 2024

Thanks for taking the time to raise this. It looks very similar to #430, but there are some differences.

I would ask the same questions here:

  • Do we need to bump the minimum version of the google auth to support this?
  • Can you verify tests are passing locally for you?

Hi, about the first question, which is the min version of the google auth?, its not in the requierements.
About the tests i get an error with aws credential but i didnt touch that, you know that could be?
FAILED dask_cloudprovider/aws/tests/test_helper.py::test_aws_to_dict_and_back - ImportError: Dask Cloud Provider AWS requirements are not installed. FAILED dask_cloudprovider/tests/test_imports.py::test_imports - ImportError: Dask Cloud Provider AWS requirements are not installed.
I installed the requirements from the project, but fail that two tests

@jacobtomlinson
Copy link
Member

Did you run pip install -e .[all] to install the optional dependencies of each cloud provider?

…ing credentials without obtaining from the environment
@RubenBBlazquez RubenBBlazquez force-pushed the feature/adapt-gcp-cluster-to-add-the-posibility-to-pass-gcp-auth-json branch from 093042e to 6a6d087 Compare August 28, 2024 12:13
@RubenBBlazquez
Copy link
Contributor Author

Did you run pip install -e .[all] to install the optional dependencies of each cloud provider?

about the version of the google-auth, the method exists in the 1.23.0 version, so theres no problem and works well
about the tests I use the command you mention , and installed all the cloud dependencies, but when i try to start the gcp tests and using the --create-external-resources option, most of the tests skip anyways, only one executes

@jacobtomlinson
Copy link
Member

That's fine. If you don't have credentials set up it will skip the test. I would expect the GCP tests to run and pass given that you're touching GCP here.

@RubenBBlazquez
Copy link
Contributor Author

RubenBBlazquez commented Aug 29, 2024

Hi, the code modified in this pr works well in the gcp tests, when i launch the tests the cluster is up correctly (if the gcpCluster connect with gcp is all i modified here and what could fail), and i cant test all of them, they take to much time to execute and finalize, so, i cant afford having the cluster up that time

@jacobtomlinson
Copy link
Member

That's fine. I appreciate you testing the modified code locally.

@jacobtomlinson jacobtomlinson merged commit 4753d3c into dask:main Aug 29, 2024
3 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants