microsoft / AzureTRE

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

Expose FIREWALL_SKU as environment variable & support start/stop of SKU Basic Firewall #3975

Closed jonnyry closed 1 week ago

jonnyry commented 3 weeks ago

Resolves #3961

What is being addressed

  1. Expose a FIREWALL_SKU environment variable into config.yaml and to be set in GitHub Actions as a variable. If the variable is not present or set to an empty string, the build should default to a Standard SKU firewall.

  2. Modify the start/stop scripts (both Powershell and make tre-start/tre-stop) to allow for the stopping and starting of the Basic SKU Firewall

github-actions[bot] commented 3 weeks ago

Unit Test Results

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

Results for commit a472e36f.

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

jonnyry commented 2 weeks ago

@marrobi thanks for review. I've added the three options (Basic, Standard, Premium) with a link to the feature comparison to the docs - see most recent commit.

jonnyry commented 2 weeks ago

@tim-allen-ck has the Smoke test failed? I can't see it in the Actions tab

tim-allen-ck commented 2 weeks ago

@tim-allen-ck has the Smoke test failed? I can't see it in the Actions tab

I've run the extended tests, but they're currently failing atm, ill give you an update by the end of the day.

tim-allen-ck commented 2 weeks ago

/test-extended ad5ac13

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/9579282932 (with refid 59582203)

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

tim-allen-ck commented 2 weeks ago

/test-destroy-env

github-actions[bot] commented 2 weeks ago

Destroying PR test environment (RG: rg-tre59582203)... (run: https://github.com/microsoft/AzureTRE/actions/runs/9584624291)

github-actions[bot] commented 2 weeks ago

PR test environment destroy complete (RG: rg-tre59582203)

tim-allen-ck commented 2 weeks ago

/test ad5ac13

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running tests: https://github.com/microsoft/AzureTRE/actions/runs/9585192545 (with refid 59582203)

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

jonnyry commented 2 weeks ago

I think this PR may need recreating... I used the rebase option to pull the commit below from main on top of the PR and looks like its not applied it:

https://github.com/microsoft/AzureTRE/commit/59d006a872f14af07b423957c7b1e0f9a1bed5fd


Update - actually it does look OK, I can see the fix applied at the head of the branch:

https://github.com/nwsde/nwsde-azuretre/blob/jr/upstream-main/44-firewall-sku/templates/workspace_services/mysql/terraform/mysql.tf#L31

Though let me know if you'd rather not merge in a rebased branch as I am aware there are potential problems and I will recreate it.

@tim-allen-ck

tim-allen-ck commented 2 weeks ago

/test

github-actions[bot] commented 2 weeks 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 weeks ago

/test a472e36

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running tests: https://github.com/microsoft/AzureTRE/actions/runs/9615259632 (with refid 59582203)

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

tim-allen-ck commented 2 weeks ago

I think this PR may need recreating... I used the rebase option to pull the commit below from main on top of the PR and looks like its not applied it:

https://github.com/microsoft/AzureTRE/commit/59d006a872f14af07b423957c7b1e0f9a1bed5fd


Update - actually it does look OK, I can see the fix applied at the head of the branch:

https://github.com/nwsde/nwsde-azuretre/blob/jr/upstream-main/44-firewall-sku/templates/workspace_services/mysql/terraform/mysql.tf#L31

Though let me know if you'd rather not merge in a rebased branch as I am aware there are potential problems and I will recreate it.

@tim-allen-ck

That's okay happy to merge