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

Add http(s) scheme if missing when logging in #658

Merged
merged 3 commits into from
Sep 30, 2024

Conversation

sgillies
Copy link
Collaborator

@sgillies sgillies commented Sep 27, 2024

Usually, a hostname doesn't include a protocol scheme, but our SDK strictly requires the http(s) scheme. Thus, we add it if it is missing when logging in.

@sgillies sgillies added the bug Something isn't working label Sep 27, 2024
@sgillies sgillies self-assigned this Sep 27, 2024
@sgillies sgillies requested a review from Shelnutt2 September 27, 2024 22:58
@sgillies sgillies changed the title Add https scheme if missing when logging in Add http(s) scheme if missing when logging in Sep 27, 2024
@sgillies
Copy link
Collaborator Author

sgillies commented Sep 30, 2024

The worker that ran tests/test_dag.py::DAGClassTest::test_kwargs is stuck. It also called tiledb.cloud.login() with a bogus host, I'll double check that I didn't screw up the process.

tiledb.cloud.config.configuration.Configuration(),
)
monkeypatch.setattr(tiledb.cloud.client.config, "logged_in", False)
monkeypatch.setattr(tiledb.cloud.client, "client", tiledb.cloud.client.Client())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's the fix. A reversible login is tricky to do.

@sgillies sgillies merged commit eacac73 into main Sep 30, 2024
18 checks passed
@sgillies sgillies deleted the sg/sc-56351/add-missing-scheme branch September 30, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants