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

Add lib for mysql server and mysql database #160

Closed
wants to merge 12 commits into from

Conversation

mikaelkrief
Copy link
Contributor

Description

Add azurerm test librairies for:

  • mysql server + mysql servers
  • mysql server database + mysql server databases

include:

  • librairies code
  • terraform code for create mysql server and database
  • associate tests
  • documentation
  • update the Readme with doc links

@mikaelkrief mikaelkrief requested a review from a user January 13, 2019 15:21
Signed-off-by: Mikael Krief <krief_mikael@hotmail.com>
Signed-off-by: Mikael Krief <krief_mikael@hotmail.com>
@dmccown
Copy link
Contributor

dmccown commented Jan 15, 2019

Thank you for your contribution! I'll work with @r-fennell and make sure one of us gets this reviewed.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hi @mikaelkrief !

Thank you for this useful contribution. I look forward to merging this and adding the new functionality you have introduced. I've left a few minor change requests - if there is anything you require clarification or assistance with feel free to reply directly or reach out to me on the Chef Community Slack.

Ruairi

'az', 'account',
'set',
'--subscription', ENV['AZURE_SUBSCRIPTION_ID']
)
Copy link

Choose a reason for hiding this comment

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

👍 - Useful for those juggling multiple AZ accounts.

Rakefile Outdated
@@ -131,7 +137,7 @@ namespace :test do
task :integration, [:controls] => [:check_attributes_file] do |_t, args|
cmd = %W( bin/inspec exec test/integration/verify
--attrs terraform/#{ENV['ATTRIBUTES_FILE']}
--reporter progress
--reporter cli
Copy link

Choose a reason for hiding this comment

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

Can I ask the reasoning behind this? We switched to the progress reporter because generally we only want to know when something has gone wrong, and the cli reporter can be very verbose (especially in CI logs).
I'd rather stick to progress unless you have found a pressing need for it.

@@ -117,6 +117,9 @@ To determine which properties are available for a given resource, start by looki

## Create documentation in `docs/resources`
Once everything is working, documentation must be added for the resources that have been added. Copy similar resource documents in `docs/resources/` and edit them as appropriate. Include enough examples to give a good idea how the resource works. Make sure to include any special case examples that might exist.
After writing the documentation:
- Run `bundle exec rake docs:resource_links`
- Copy/Paste all display links in the Readme.md
Copy link

Choose a reason for hiding this comment

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

👍


### Version

This resource first became available in 1.2.0 of the inspec-azure resource pack.
Copy link

Choose a reason for hiding this comment

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

This can be bumped to 1.3.0 - we will release after this addition.


### Version

This resource first became available in 1.2.0 of the inspec-azure resource pack.
Copy link

Choose a reason for hiding this comment

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

This can be bumped to 1.3.0 - we will release after this addition.

docs/resources/azurerm_mysql_servers.md.erb Outdated Show resolved Hide resolved
@@ -4,6 +4,7 @@ class EnvironmentFile
network_watcher
msi
sql
mysql
Copy link

Choose a reason for hiding this comment

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

This is fine to bundle with the sql option. You can update your controls to only_if { ENV['SQL'] }

resource_group_name = "${azurerm_resource_group.rg.name}"

sku {
name = "${var.sku_name}"
Copy link

Choose a reason for hiding this comment

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

If the variables being referenced here (in variables.tf) are not reused, we should simply hardcode the values in azure.tf until they're needed in multiple places. I dont expect we will need to reuse any the MySQL values.

@@ -73,3 +73,71 @@ variable "public_vm_count" {
variable "sql-server-version" {
default = "12.0"
}


# Azure mysql
Copy link

Choose a reason for hiding this comment

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

As above, it's fine to hardcode most/all of these straight into the terraform resource, unless they are being used in several places!


describe azurerm_mysql_database(resource_group: resource_group,
server_name: mysql_server_name,
database_name: mysql_db_name) do
Copy link

Choose a reason for hiding this comment

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

Tab alignment :)

@mikaelkrief
Copy link
Contributor Author

@r-fennell Thanks, I fixed and pushed all this feedbacks

@mikaelkrief mikaelkrief requested a review from a team May 8, 2019 18:56
@ghost
Copy link

ghost commented Jul 11, 2019

Hi,
I'm closing this PR as a duplicate of #161 which builds on this work.
Thanks @mikaelkrief !

@ghost ghost closed this Jul 11, 2019
This pull request was closed.
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.

2 participants