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.

Sign up for GitHub

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

Jump to bottom

Add support for LINC staging and production APIs #1519

Closed
wants to merge 9 commits into from
Closed

Add support for LINC staging and production APIs #1519

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6d23a4b
Support LINC staging and production APIs in dandi-cli tooling
Nov 5, 2024
fce9ca9
temp
Nov 7, 2024
979c076
Resolve failing unit tests
Nov 8, 2024
5071abb
remove venv
Nov 8, 2024
f5a6f1f
Trivial change to invoke CI tests again
Nov 12, 2024
5628bc1
disable versioning for now
Nov 12, 2024
45360f9
remove blocking logic
Nov 12, 2024
3e258a4
remove is_private kwarg
Nov 12, 2024
aa080a1
update per yarik's comment
Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions dandi/cli/tests/test_instances.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,21 @@ def test_cmd_instances(monkeypatch):
"dandi:\n"
" api: https://api.dandiarchive.org/api\n"
" gui: https://dandiarchive.org\n"
" is_private: null\n"
"dandi-api-local-docker-tests:\n"
f" api: http://{instancehost}:8000/api\n"
f" gui: http://{instancehost}:8085\n"
" is_private: null\n"
"dandi-staging:\n"
" api: https://api-staging.dandiarchive.org/api\n"
" gui: https://gui-staging.dandiarchive.org\n"
" is_private: null\n"
"linc:\n"
" api: https://api.lincbrain.org/api\n"
" gui: https://lincbrain.org\n"
" is_private: true\n"
"linc-staging:\n"
" api: https://staging-api.lincbrain.org/api\n"
" gui: https://staging.lincbrain.org\n"
" is_private: true\n"
)
15 changes: 15 additions & 0 deletions dandi/consts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
from enum import Enum
import os

from typing import Optional
github-advanced-security[bot] marked this conversation as resolved.
Fixed Show resolved Hide resolved

#: A list of metadata fields which dandi extracts from .nwb files.
#: Additional fields (such as ``number_of_*``) might be added by
#: `get_metadata()`
Expand Down Expand Up @@ -100,6 +102,7 @@ class DandiInstance:
name: str
gui: str | None
api: str
is_private: Optional[bool] = None
yarikoptic marked this conversation as resolved.
Show resolved Hide resolved
yarikoptic marked this conversation as resolved.
Show resolved Hide resolved

@property
def redirector(self) -> None:
Expand Down Expand Up @@ -132,6 +135,18 @@ def urls(self) -> Iterator[str]:
f"http://{instancehost}:8085",
f"http://{instancehost}:8000/api",
),
"linc": DandiInstance(
"linc",
"https://lincbrain.org",
"https://api.lincbrain.org/api",
is_private=True
),
"linc-staging": DandiInstance(
"linc-staging",
"https://staging.lincbrain.org",
Copy link
Member

Choose a reason for hiding this comment

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

Hi @aaronkanzer, it looks like https://staging.lincbrain.org isn't able to resolve again. Not sure if we changed anything. See screenshot.

image

Copy link
Member Author

@aaronkanzer aaronkanzer Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Screenshot 2024-11-12 at 12 24 18 PM

@kabilar -- it seems that SSL support isn't provided out-of-the-box for the branch deploys URLs in Netlify

Firefox and Chrome require the SSL enforcement, but if you use Safari, you can proceed without the SSL -- didn't want to disturb the peace here with changing the SSL cert, so didn't update anything for now

"https://staging-api.lincbrain.org/api",
is_private=True
)
}
# to map back url: name
known_instances_rev = {
Expand Down
22 changes: 11 additions & 11 deletions dandi/dandiapi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,17 +649,17 @@
schema_version = models.get_schema_version()
server_info = self.get("/info/")
server_schema_version = server_info.get("schema_version")
if not server_schema_version:
raise RuntimeError(
"Server did not provide schema_version in /info/;"
f" returned {server_info!r}"
)
if server_schema_version != schema_version:
raise SchemaVersionError(
f"Server requires schema version {server_schema_version};"
f" client only supports {schema_version}. You may need to"
" upgrade dandi and/or dandischema."
)
# if not server_schema_version:
Copy link
Member

Choose a reason for hiding this comment

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

why this block is commented out -- doesn't LINC provide /info/ with schema_version?

Copy link
Member Author

Choose a reason for hiding this comment

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

When I was testing, my forked branch is technically tagged as a version of 0+untagged<some-sha-here> thus I commented this out for testing purposes.

As noted in #1519 (comment) -- this will be commented back in once you et. al are happy with the core changes here

# raise RuntimeError(
# "Server did not provide schema_version in /info/;"
# f" returned {server_info!r}"
Comment on lines +652 to +655

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
Comment on lines +652 to +655
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# if not server_schema_version:
# raise RuntimeError(
# "Server did not provide schema_version in /info/;"
# f" returned {server_info!r}"
if not server_schema_version:
raise RuntimeError(
"Server did not provide schema_version in /info/;"
f" returned {server_info!r}"

# )
# if server_schema_version != schema_version:
# raise SchemaVersionError(
# f"Server requires schema version {server_schema_version};"
# f" client only supports {schema_version}. You may need to"
# " upgrade dandi and/or dandischema."
Comment on lines +657 to +661

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment for reference in another issue

# )
Comment on lines +656 to +662
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# )
# if server_schema_version != schema_version:
# raise SchemaVersionError(
# f"Server requires schema version {server_schema_version};"
# f" client only supports {schema_version}. You may need to"
# " upgrade dandi and/or dandischema."
# )
)
if server_schema_version != schema_version:
raise SchemaVersionError(
f"Server requires schema version {server_schema_version};"
f" client only supports {schema_version}. You may need to"
" upgrade dandi and/or dandischema."
)


def get_asset(self, asset_id: str) -> BaseRemoteAsset:
"""
Expand Down
2 changes: 1 addition & 1 deletion dandi/tests/test_dandiarchive.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ def test_parse_dandi_url_unknown_instance() -> None:
parse_dandi_url("dandi://not-an-instance/000001")
assert str(excinfo.value) == (
"Unknown instance 'not-an-instance'. Valid instances: dandi,"
" dandi-api-local-docker-tests, dandi-staging"
" dandi-api-local-docker-tests, dandi-staging, linc, linc-staging"
Copy link
Member

Choose a reason for hiding this comment

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

might be worth right away to compose this from known_instances to avoid hardcoding. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just the unit test case -- in the prod. code it isn't hard-coding

Did you have in-mind the hard-coding being removed in the unit test here as well?

Copy link
Member

Choose a reason for hiding this comment

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

yeap. I see no reason to hardcode them in the test here. The purpose is to test to see that all are listed in the particular order. Code could be used to provide that and avoid future edits happen we need to add one more instance

Copy link
Member

Choose a reason for hiding this comment

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

on this note, chime in to

Copy link
Member Author

Choose a reason for hiding this comment

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

yeap. I see no reason to hardcode them in the test here. The purpose is to test to see that all are listed in the particular order. Code could be used to provide that and avoid future edits happen we need to add one more instance

Updated the test here

)


Expand Down
28 changes: 14 additions & 14 deletions dandi/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,19 +604,19 @@
f"Could not retrieve server info from {url},"
" and client does not recognize URL"
)
try:
minversion = Version(server_info.cli_minimal_version)
bad_versions = [Version(v) for v in server_info.cli_bad_versions]
except ValueError as e:
raise ValueError(
f"{url} returned an incorrectly formatted version;"
f" please contact that server's administrators: {e}"
)
our_version = Version(__version__)
if our_version < minversion:
raise CliVersionTooOldError(our_version, minversion, bad_versions)
if our_version in bad_versions:
raise BadCliVersionError(our_version, minversion, bad_versions)
# try:
# minversion = Version(server_info.cli_minimal_version)
# bad_versions = [Version(v) for v in server_info.cli_bad_versions]
# except ValueError as e:
# raise ValueError(
# f"{url} returned an incorrectly formatted version;"
# f" please contact that server's administrators: {e}"
Comment on lines +607 to +613

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
# )
# our_version = Version(__version__)
# if our_version < minversion:
# raise CliVersionTooOldError(our_version, minversion, bad_versions)
# if our_version in bad_versions:
# raise BadCliVersionError(our_version, minversion, bad_versions)
Comment on lines +616 to +619

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
api_url = server_info.services.api.url
if dandi_id is None:
# Don't use pydantic.AnyHttpUrl, as that sets the `port` attribute even
Expand All @@ -642,7 +642,7 @@


def is_url(s: str) -> bool:
"""Very primitive url detection for now
"""Very primitive url detection

TODO: redo
"""
Expand Down
6 changes: 6 additions & 0 deletions docs/source/cmdline/instances.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,9 @@ Example output:
dandi-staging:
api: https://api-staging.dandiarchive.org/api
gui: https://gui-staging.dandiarchive.org
linc-staging:
api: https://staging-api.lincbrain.org/api
gui: https://staging.lincbrain.org
linc:
api: https://api.lincbrain.org/api
gui: https://lincbrain.org
Loading