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.

Sign up for GitHub

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

Jump to bottom

Guacamole user resource templates tagging #2061

Merged
merged 22 commits into from
Jun 24, 2022
Merged

Guacamole user resource templates tagging #2061

merged 22 commits into from
Jun 24, 2022

Conversation

pedro-pelegrin-nttdata
Copy link
Contributor

What is being addressed

As a TRE developer,
I want to tag existing user resources templates
So that these templates include information for TRE id, TRE Workspace id, TRE Workspace Service Id and TRE User Resource id.

How is this addressed

  • Added tre_user_resources_tags to locals.tf
  • Added variable tre_user_resources_tags to variables.tf
  • Added tags to tf files

@github-actions github-actions bot added the external PR from an external contributor label Jun 20, 2022
@pedro-pelegrin-nttdata pedro-pelegrin-nttdata marked this pull request as ready for review June 20, 2022 08:16
@joalmeid joalmeid changed the title Pedro-pelegrin-nttdata/3862_Tag_azureml_Guacamole_user_resource_templates Guacamole user resource templates tagging Jun 20, 2022
@joalmeid joalmeid linked an issue Jun 20, 2022 that may be closed by this pull request
Tag azureml and Guacamole user resource templates in Terraform #1897
Closed
8 tasks
@guybartal
Copy link
Contributor

/test-extended 0a47a85

@github-actions
Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/2531398263 (with refid c4ee59a6)

(in response to this comment from @guybartal)

@guybartal
Copy link
Contributor

/test-extented cdda9ed

@github-actions
Copy link

🤖 pr-bot 🤖

/test-extented 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 @guybartal)

@guybartal
Copy link
Contributor

/test-extended 6d23d6e

@github-actions
Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/2535655961 (with refid c4ee59a6)

(in response to this comment from @guybartal)

guybartal
guybartal suggested changes Jun 21, 2022
View reviewed changes
Copy link
Contributor

@guybartal guybartal left a comment

Choose a reason for hiding this comment

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

please make sure you use the correct variables for tags,
it would be great if you'll try to deploy it on azure using the TRE API to make sure you see all required tags on all resources

...ates/workspace_services/guacamole/user_resources/guacamole-azure-linuxvm/terraform/locals.tf Outdated Show resolved Hide resolved
...es/workspace_services/guacamole/user_resources/guacamole-azure-windowsvm/terraform/locals.tf Outdated Show resolved Hide resolved
templates/workspace_services/guacamole/user_resources/guacamole-dev-vm/terraform/locals.tf Outdated Show resolved Hide resolved
@guybartal
Copy link
Contributor

/test-extended c5d9c6a

@github-actions
Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/2547931757 (with refid c4ee59a6)

(in response to this comment from @guybartal)

guybartal
guybartal suggested changes Jun 23, 2022
View reviewed changes
Copy link
Contributor

@guybartal guybartal left a comment

Choose a reason for hiding this comment

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

can you please try to create a VM and make sure all correct tags are set on Azure?

templates/workspace_services/azureml/user_resources/aml_compute/terraform/locals.tf Outdated Show resolved Hide resolved
templates/workspace_services/azureml/user_resources/aml_compute/terraform/locals.tf Outdated Show resolved Hide resolved
guybartal
guybartal approved these changes Jun 23, 2022
View reviewed changes
Copy link
Contributor

@guybartal guybartal left a comment

Choose a reason for hiding this comment

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

LGTM

@guybartal
Copy link
Contributor

/test-extended 43a69fd

@github-actions
Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/2549491449 (with refid c4ee59a6)

(in response to this comment from @guybartal)

tamirkamara
tamirkamara requested changes Jun 24, 2022
View reviewed changes
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.

@pedro-pelegrin-nttdata there's an error deploying these resources as you've added a variable to the root module. Please see my comments.

templates/workspace_services/azureml/user_resources/aml_compute/terraform/variables.tf Outdated Show resolved Hide resolved
...tes/workspace_services/guacamole/user_resources/guacamole-azure-linuxvm/terraform/linuxvm.tf Outdated Show resolved Hide resolved
...s/workspace_services/guacamole/user_resources/guacamole-azure-linuxvm/terraform/variables.tf Outdated Show resolved Hide resolved
...workspace_services/guacamole/user_resources/guacamole-azure-windowsvm/terraform/variables.tf Outdated Show resolved Hide resolved
...workspace_services/guacamole/user_resources/guacamole-azure-windowsvm/terraform/windowsvm.tf Outdated Show resolved Hide resolved
templates/workspace_services/guacamole/user_resources/guacamole-dev-vm/terraform/linuxvm.tf Outdated Show resolved Hide resolved
templates/workspace_services/guacamole/user_resources/guacamole-dev-vm/terraform/variables.tf Outdated Show resolved Hide resolved
@tamirkamara
Copy link
Collaborator

/test-extended 109f31a

@github-actions
Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/2554959810 (with refid c4ee59a6)

(in response to this comment from @tamirkamara)

tamirkamara
tamirkamara approved these changes Jun 24, 2022
View reviewed changes
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.

Great. Tests pass as well.

@tamirkamara tamirkamara merged commit bff9a7e into microsoft:main Jun 24, 2022
@pedro-pelegrin-nttdata pedro-pelegrin-nttdata deleted the pedro-pelegrin-nttdata/3862_Tag_azureml_Guacamole_user_resource_templates branch June 24, 2022 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tamirkamara tamirkamara tamirkamara approved these changes

@guybartal guybartal guybartal approved these changes

Assignees
No one assigned
Labels
external PR from an external contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tag azureml and Guacamole user resource templates in Terraform
4 participants
@pedro-pelegrin-nttdata @guybartal @tamirkamara @ross-p-smith