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.

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

Pipeline Property Substitution #2052

Merged
merged 24 commits into from
Jun 20, 2022
Merged

Conversation

damoodamoo
Copy link
Member

Closes #1679

What is being addressed

This PR adds support for a template designer to update a secondary resource from a primary, using properties from the primary in the upgrade.

To do this, a designer can use the syntax {{ resource.properties.prop_name }} , which will be translated at runtime to the real value. Using this method, Nexus + Gitea (in this PR) now update the firewall as a secondary resource rather than directly via their own terraform. Since we have queueing in place it should mean that Gitea + Nexus can be deployed in parallel more safely.

To support this the way outputs are returned from bundles into the cosmos object has been updated. Rather than converting every output to a string in the resource, we now allow a property bag to contain outputs of complex types.

In the UI, the notifications panel will show the pipeline steps such:
image

@damoodamoo damoodamoo changed the title Pipeline Substitution Pipeline Property Substitution Jun 16, 2022
@github-actions
Copy link

github-actions bot commented Jun 16, 2022

Unit Test Results

288 tests  +287   286 ✔️ +285   9s ⏱️ - 31m 24s
    1 suites ±    0       2 💤 +    2 
    1 files   ±    0       0 ±    0 

Results for commit 0c60bf7. ± Comparison against base commit 16f4c60.

This pull request removes 1 and adds 288 tests. Note that renamed tests count towards both.
test_workspace_services ‑ test_create_guacamole_service_into_aad_workspace
tests_ma.test_api.test_errors.test_422_error ‑ test_frw_validation_error_format
tests_ma.test_api.test_errors.test_error ‑ test_frw_validation_error_format
tests_ma.test_api.test_routes.test_api_access.TestTemplateRoutesThatRequireAdminRights ‑ test_get_workspace_templates_requires_admin_rights
tests_ma.test_api.test_routes.test_api_access.TestTemplateRoutesThatRequireAdminRights ‑ test_post_user_resource_templates_requires_admin_rights
tests_ma.test_api.test_routes.test_api_access.TestTemplateRoutesThatRequireAdminRights ‑ test_post_workspace_service_templates_requires_admin_rights
tests_ma.test_api.test_routes.test_api_access.TestTemplateRoutesThatRequireAdminRights ‑ test_post_workspace_templates_requires_admin_rights
tests_ma.test_api.test_routes.test_api_access.TestUserResourcesOwnerOrResearcherRoutesAccess ‑ test_delete_user_resource_raises_403_if_user_is_not_workspace_owner_or_researcher
tests_ma.test_api.test_routes.test_api_access.TestUserResourcesOwnerOrResearcherRoutesAccess ‑ test_get_user_resource_raises_403_if_user_is_not_workspace_owner_or_researcher
tests_ma.test_api.test_routes.test_api_access.TestUserResourcesOwnerOrResearcherRoutesAccess ‑ test_get_user_resources_raises_403_if_user_is_not_researcher_or_owner_of_workspace
tests_ma.test_api.test_routes.test_api_access.TestUserResourcesOwnerOrResearcherRoutesAccess ‑ test_patch_user_resource_raises_403_if_user_is_not_workspace_owner_or_researcher
…

♻️ This comment has been updated with latest results.

@damoodamoo damoodamoo requested a review from marrobi June 16, 2022 15:22
@damoodamoo
Copy link
Member Author

/test-extended

@github-actions
Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/2510104310 (with refid 4e00469a)

(in response to this comment from @damoodamoo)

@damoodamoo
Copy link
Member Author

/test-destroy-env

@github-actions
Copy link

Destroying branch test environment (RG: rg-trec7302e0e)... (run: https://github.com/microsoft/AzureTRE/actions/runs/2510319711)

@github-actions
Copy link

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

@github-actions
Copy link

Branch test environment destroy complete (RG: rg-trec7302e0e)

@github-actions
Copy link

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

@damoodamoo
Copy link
Member Author

/test-extended

@github-actions
Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/2510424861 (with refid 4e00469a)

(in response to this comment from @damoodamoo)

@marrobi
Copy link
Member

marrobi commented Jun 16, 2022

Love it. Might be worth someone looking over the code - not sure I can give it the service it deserves.

Would be good to know what is/isn't supported - for example could I upgrade a workspace with a new property?

@ross-p-smith
Copy link
Contributor

Would be good to know what is/isn't supported - for example could I upgrade a workspace with a new property?

The docs say only "shared-services" are supported at the moment.

damoodamoo and others added 5 commits June 17, 2022 12:34
Co-authored-by: Ross Smith <ross-p-smith@users.noreply.github.com>
Co-authored-by: Ross Smith <ross-p-smith@users.noreply.github.com>
Co-authored-by: Ross Smith <ross-p-smith@users.noreply.github.com>
Co-authored-by: Ross Smith <ross-p-smith@users.noreply.github.com>
Copy link
Collaborator

@tamirkamara tamirkamara left a comment

Choose a reason for hiding this comment

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

I didn't review the logic and would only comment:

  1. If gitea/nexues don't need azcli anymore, can you please delete the dockerfile.tmpl as well (from the porter.yaml too)
  2. Would merging still make it possible to use the "old" firewall script (as we do so elsewhere), or does it need to be upgrades in the other places immediately?

@damoodamoo
Copy link
Member Author

@tamirkamara

  1. If gitea/nexues don't need azcli anymore, can you please delete the dockerfile.tmpl as well (from the porter.yaml too)

I have only removed the firewall terraform resources in these bundles, nothing else has changed so no change to tooling needed.

  1. Would merging still make it possible to use the "old" firewall script (as we do so elsewhere), or does it need to be upgrades in the other places immediately?

By 'old' firewall script do you mean the existing one in main? It's still possible for other resources to reference the firewall as before, and existing things should continue to work. It's a good point though, and what i'll do is rename the rules that gitea + nexus now create to avoid clashes with any existing implementations.

Copy link
Contributor

@ross-p-smith ross-p-smith left a comment

Choose a reason for hiding this comment

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

Fantastico!! What a PR ❤️

@damoodamoo
Copy link
Member Author

/test-destroy-env

@github-actions
Copy link

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

@github-actions
Copy link

Destroying branch test environment (RG: rg-trec7302e0e)... (run: https://github.com/microsoft/AzureTRE/actions/runs/2527522640)

@github-actions
Copy link

Branch test environment destroy complete (RG: rg-trec7302e0e)

@github-actions
Copy link

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

@damoodamoo
Copy link
Member Author

/test-extended

@github-actions
Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/2527629989 (with refid 4e00469a)

(in response to this comment from @damoodamoo)

@damoodamoo
Copy link
Member Author

/test-shared-services

@github-actions
Copy link

🤖 pr-bot 🤖

🏃 Running shared service tests: https://github.com/microsoft/AzureTRE/actions/runs/2528324609 (with refid 4e00469a)

(in response to this comment from @damoodamoo)

@damoodamoo
Copy link
Member Author

/test-shared_services

@github-actions
Copy link

🤖 pr-bot 🤖

/test-shared_services is not recognised as a valid command.

You can use the following commands:
    /test - build, deploy and run smoke tests on a PR
    /test-extended - build, deploy and run smoke & extended tests on a PR
    /test-shared-services - test the deployment of shared services on a PR build
    /test-force-approve - force approval of the PR tests (i.e. skip the deployment checks)
    /test-destroy-env - delete the validation environment for a PR (e.g. to enable testing a deployment from a clean start after previous tests)
    /help - show this help

(in response to this comment from @damoodamoo)

@damoodamoo damoodamoo merged commit 9984e5e into main Jun 20, 2022
@damoodamoo damoodamoo deleted the damoo/1679-pipeline-substitution branch June 20, 2022 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Substitutions Mechanism: MVP
4 participants