microsoft / AzureTRE

An accelerator to help organizations build Trusted Research Environments on Azure.
https://microsoft.github.io/AzureTRE
MIT License
182 stars 139 forks source link

Azure SQL Workspace Service #3970

Closed jonnyry closed 2 months ago

jonnyry commented 3 months ago

Changes

New Azure SQL workspace service, based on existing MySQL workspace

Resolves #3969

What is being addressed

Adds an Azure SQL workspace service following the same pattern as the My SQL workspace service.

Follow ups

A couple of follow up changes are required after this PR:

1. The following repo is referenced in several places in the code https://github.com/microsoft/terraform-azurerm-environment-configuration.git. Previously this was suffixed with ?ref=x.x.x, however I am waiting on a contributor to create a release so I can re-instate the reference. This has been marked with the following comment in the code: # add specific ref once release 0.5.0 has been created (now completed - see commit dated 18-Jul)

  1. Workspaces derived from base (i.e. unrestricted and airlock-import-review) both pull in the base workspace code from GitHub again using a ref. Once a release has been created incorporating these changes, this ref needs updating in both workspace templates in the Dockerfile.tmpl file, variable AZURE_TRE_VERSION
github-actions[bot] commented 3 months ago

Unit Test Results

0 tests   0 :white_check_mark:  0s :stopwatch: 0 suites  0 :zzz: 0 files    0 :x:

Results for commit d0ad3d53.

:recycle: This comment has been updated with latest results.

jonnyry commented 3 months ago

Hi @marrobi @tim-allen-ck possible to get a review/merge on this one? Thanks

marrobi commented 3 months ago

Hi @marrobi @tim-allen-ck possible to get a review/merge on this one? Thanks

Tim focused on getting the release out last week. Will will make sure we test this out in the next week or so. Is it not being merged blocking you in any way?

jonnyry commented 3 months ago

Hi @marrobi @tim-allen-ck possible to get a review/merge on this one? Thanks

Tim focused on getting the release out last week. Will will make sure we test this out in the next week or so. Is it not being merged blocking you in any way?

Not strictly, as I can deploy from a branch with the changes in. But preferable to have integrated into main.

marrobi commented 3 months ago

Hi @marrobi @tim-allen-ck possible to get a review/merge on this one? Thanks

Tim focused on getting the release out last week. Will will make sure we test this out in the next week or so. Is it not being merged blocking you in any way?

Not strictly, as I can deploy from a branch with the changes in. But preferable to have integrated into main.

Ok, you can also add the bundle into your copy of the deployment repo.

tim-allen-ck commented 3 months ago

Hi @marrobi @tim-allen-ck possible to get a review/merge on this one? Thanks

I've left some comments on the code to look at.

jonnyry commented 3 months ago

Hi @marrobi @tim-allen-ck possible to get a review/merge on this one? Thanks

I've left some comments on the code to look at.

Hi @tim-allen-ck Thanks, though I can't see any? I might be looking in the wrong place.

jonnyry commented 2 months ago

@tim-allen-ck hey tim - I can't see the review comments you mentioned yesterday? happy to demo this one if you like - hopefully more succesfully than yesterday :-D

tim-allen-ck commented 2 months ago

@tim-allen-ck hey tim - I can't see the review comments you mentioned yesterday? happy to demo this one if you like - hopefully more succesfully than yesterday :-D

Hey, was just wanting you to use the most recent tf providers and an optional password field.

jonnyry commented 2 months ago

Hey, was just wanting you to use the most recent tf providers and an optional password field.

I've updated the terraform version & provider versions.

Re the optional password field - could I suggest that we open a separate ticket for that which covers both MySQL & AzureSQL (and potentially looks at other auth options)? I've gone with a straight conversion of the existing MySQL template to Azure SQL and not wanting to add new features in this PR.

tim-allen-ck commented 2 months ago

/test-force-approve

github-actions[bot] commented 2 months ago

:robot: pr-bot :robot:

:white_check_mark: Marking tests as complete (for commit 998070eb311861c196172964f0974b7f7c80ab1b)

(in response to this comment from @tim-allen-ck)

tim-allen-ck commented 2 months ago

/test

github-actions[bot] commented 2 months ago

:robot: pr-bot :robot:

:warning: When using /test on external PRs, the SHA of the checked commit must be specified

(in response to this comment from @tim-allen-ck)

tim-allen-ck commented 2 months ago

/test d0ad3d5

github-actions[bot] commented 2 months ago

:robot: pr-bot :robot:

:runner: Running tests: https://github.com/microsoft/AzureTRE/actions/runs/9935850439 (with refid 12db64b2)

(in response to this comment from @tim-allen-ck)