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

azure-vm-simple-linux #8

Closed

Conversation

scottzilla
Copy link
Collaborator

Terraform HCL version of ARM 101-vm-simple-linux quickstart template
CI enabled.

anniehedgpeth and others added 30 commits April 17, 2017 15:46
@scottzilla
Copy link
Collaborator Author

@harijayms once we're accepted, I can handle squashing the commit for cleaner history

@paulbouwer
Copy link
Collaborator

@scottzilla @harijayms @echuvyrov - Is this a duplicate of the work on examples/azure/101-vm-simple-linux in PR #6 ?

It may be worth combining them and ensuring that we are placing all these examples into the examples/azure folder in the upstream repo.

@cloudbooster
Copy link
Owner

@paulbouwer @echuvyrov I thought your sample was with native disk, this one is with the managed disk. We should name accordingly.

@scottzilla
Copy link
Collaborator Author

@harijayms if we renamed this to be: azure-vm-simple-linux-managed-disk, I believe we would be consistent with the quickstart naming

@scottzilla
Copy link
Collaborator Author

Also, there are a few additional files purely for enabling CI of this example and are not intended to be pushed upstream:
deploy.sh
after_deploy.sh

@cloudbooster
Copy link
Owner

We can push deploy.sh and after_deploy.sh as well. I feel this would be good for customers as well.

The only thing that I will add is make the docker engine download as a part of it, or check for docker presence instead of failing catastrophically

Copy link
Owner

@cloudbooster cloudbooster left a comment

Choose a reason for hiding this comment

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

I have added comments , please address, once done I think you can merge and push this to terraform.


storage_os_disk {
name = "${var.hostname}-osdisk1"
vhd_uri = "${azurerm_storage_account.stor.primary_blob_endpoint}${azurerm_storage_container.storc.name}/${var.hostname}-osdisk1.vhd"
Copy link
Owner

Choose a reason for hiding this comment

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

I thought we didnt need to specify VHD URI for Managed disk ,


storage_data_disk {
name = "${var.hostname}-disk2"
vhd_uri = "${azurerm_storage_account.stor.primary_blob_endpoint}${azurerm_storage_container.storc.name}/${var.hostname}-disk2.vhd"
Copy link
Owner

Choose a reason for hiding this comment

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

Same here?


# cleanup deployed azure resources
docker run --rm -it \
azuresdk/azure-cli-python \
Copy link
Owner

Choose a reason for hiding this comment

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

We should use the az cli to validate that the resource was created or add that in deploy.sh , how would we know if the resource was created?

/bin/terraform validate; \
/bin/terraform plan -out=out.tfplan -var dns_name=$KEY -var hostname=$KEY -var resource_group=$KEY -var admin_password=$PASSWORD; \
/bin/terraform apply out.tfplan"

Copy link
Owner

Choose a reason for hiding this comment

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

Add terraform show here, it will be good to record it in the logs

}

resource "azurerm_storage_container" "storc" {
name = "${var.hostname}-vhds"
Copy link
Owner

Choose a reason for hiding this comment

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

We dont need this container now right?

@scottzilla
Copy link
Collaborator Author

@harijayms
Good catch; removed storage account container.
Added terraform show after apply.

}

variable "storage_account_type" {
description = "Specifies the name of the storage account. Changing this forces a new resource to be created. This must be unique across the entire Azure service, not just within the resource group."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Description is wrong

}

variable "vm_size" {
description = "Specifies the name of the virtual machine resource. Changing this forces a new resource to be created."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrong description

@scottzilla
Copy link
Collaborator Author

Thank you @StephenWeatherford, the descriptions have been updated accordingly.

@cloudbooster
Copy link
Owner

@scottzilla I dont see the CI running terraform for this, what am I missing?

@scottzilla
Copy link
Collaborator Author

scottzilla commented Apr 27, 2017

@harijayms
Added an az vm show ... to verify that the VM resource has actually been created outside the domain of terraform.

@scottzilla
Copy link
Collaborator Author

@scottzilla scottzilla closed this Apr 28, 2017
@scottzilla scottzilla deleted the topic-101-vm-simple-linux branch April 28, 2017 01:19
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.

5 participants