Private AML: Dockerfile with Nexus config #3424
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Unit Test Results0 tests 0 ✅ 0s ⏱️ Results for commit a8dfe47. ♻️ This comment has been updated with latest results. |
jjgriff93
approved these changes
Apr 6, 2023
martinpeck
reviewed
Apr 6, 2023
|
|
||
| ARG TRE_ID | ||
| ARG TRE_LOCATION | ||
| ENV NEXUS_PROXY_URL="https://nexus-${TRE_ID}.${TRE_LOCATION}.cloudapp.azure.com" |
There was a problem hiding this comment.
is this going to need to be updated as per the Gov cloud stuff?
There was a problem hiding this comment.
yep - have updated to set the endpoint based on cloud type
| resource "azurerm_container_registry" "acr" { | ||
| name = local.acr_name | ||
| location = data.azurerm_resource_group.ws.location | ||
| resource_group_name = data.azurerm_resource_group.ws.name | ||
| sku = "Premium" | ||
| admin_enabled = false | ||
| public_network_access_enabled = false | ||
| public_network_access_enabled = var.public_access_enabled |
There was a problem hiding this comment.
as discussed, I suspect this might benefit from a comment
There was a problem hiding this comment.
agreed - have added comment to explain
|
Holding off until @tamirkamara / the crew are back from leave to review from a Gov perspective |
tamirkamara
requested changes
Apr 18, 2023
| @@ -55,3 +63,7 @@ output "mcr_tag" { | |||
| output "batch_tag" { | |||
| value = data.azurerm_network_service_tags.batch_tag.id | |||
| } | |||
|
|
|||
| output "azure_endpoint" { | |||
| value = var.azure_environment == "AzureGovCloud" ? "cloudapp.usgovcloudapi.net" : "cloudapp.azure.com" | |||
There was a problem hiding this comment.
- We have a similar condition somewhere else already... but at any case, the suffixes should be brought from TF configuration module we use elsewhere (we might need to add those as outputs there).
- If we need to have this condition in the definition, then I think we'd prefer to use
arm_environmentas that is native to Terraform.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #3295
As AML is deployed, it deploys a container registry for its own use. We need to push a customised Docker image into that registry, that contains the nexus config 'baked in', so that nexus will be used to download required packages during AML image builds during training / experimentation runs. Without the nexus config, the compute cluster can not run in network isolation.
What this PR does:
Alternatives considered and discounted: