-
Notifications
You must be signed in to change notification settings - Fork 74
Azure fixes + additional asks #468
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
Changes from all commits
aa57372
c232acb
afcabd8
3d14605
273e222
f5e4dc1
dc6d92f
67b27ef
b3efdbc
5778e48
5211e15
f62a1b5
7877fdf
7899128
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,7 +58,7 @@ def get_engine_url(env: Optional[str] = None, read_only: bool = True, sync: bool | |
| user = os.environ.get("AZURE_IDENTITY_NAME") | ||
| password = ( | ||
| DefaultAzureCredential() | ||
| .get_token("https://ossrdbms-aad.database.windows.net") | ||
| .get_token("https://ossrdbms-aad.database.windows.net/.default") | ||
| .token | ||
| ) | ||
| logger.info(f"Connecting to db {db} as user {user}") | ||
|
|
@@ -81,7 +81,9 @@ def get_engine_url(env: Optional[str] = None, read_only: bool = True, sync: bool | |
|
|
||
| # For async postgres, we need to use an async dialect. | ||
| if not sync: | ||
| engine_url = engine_url.replace("postgresql://", "postgresql+asyncpg://") | ||
| engine_url = engine_url.replace("postgresql://", "postgresql+asyncpg://").replace( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For sync, psycopg2 needs sslmode, but for async, asyncpg needs ssl. This shouldn't affect AWS because the |
||
| "sslmode", "ssl" | ||
| ) | ||
| return engine_url | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| from model_engine_server.common.dtos.docker_repository import BuildImageRequest, BuildImageResponse | ||
| from model_engine_server.core.config import infra_config | ||
| from model_engine_server.core.loggers import logger_name, make_logger | ||
| from model_engine_server.domain.exceptions import DockerRepositoryNotFoundException | ||
| from model_engine_server.domain.repositories import DockerRepository | ||
|
|
||
| logger = make_logger(logger_name()) | ||
|
|
@@ -36,7 +37,11 @@ def get_latest_image_tag(self, repository_name: str) -> str: | |
| credential = DefaultAzureCredential() | ||
| client = ContainerRegistryClient(endpoint, credential) | ||
|
|
||
| image = client.list_manifest_properties( | ||
| repository_name, order_by="time_desc", results_per_page=1 | ||
| ).next() | ||
| return image.tags[0] | ||
| try: | ||
| image = client.list_manifest_properties( | ||
| repository_name, order_by="time_desc", results_per_page=1 | ||
| ).next() | ||
| # Azure automatically deletes empty ACR repositories, so repos will always have at least one image | ||
| return image.tags[0] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we want to throw an error if there are 0 image tags?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unless that's already handled in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just tested, it looks like Azure automatically deletes repositories that are empty, so it'll be a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah ok sounds good, could we note it in the code so we know why we're not gonna |
||
| except ResourceNotFoundError: | ||
| raise DockerRepositoryNotFoundException | ||
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.
when does
file.filenameequal None, should we just throw a 4xx in that case (if it is user fault thatfile.filenamecould be None?)maybe this is fine if
filenamedoesn't get used as the only part of an identifier actually, tbh I'm not sure thoughThere 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.
Hmm not sure, I'm assuming this came up from lint because
file.filenamechanged fromstrtoOptional[str]between the old and new FastAPI versions... kinda hoping it's just always defined lol 😅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.
do we know when filename can be None? eg does the fastapi documentation say anything about it
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.
Hmm I couldn't find anything in the documentation, but it does seem like there are methods of calling where this can be user-set