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

feat: add Azure action group module #589

Merged
merged 11 commits into from
Sep 23, 2020

Conversation

tsalright
Copy link

This adds example terraform, the Azure Action Group module, and a test that used the example and the module.
This PR will help us test the work being done for #89 and is a useful module to have.

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I left a bunch of comments, but they are all minor.

However, one thing I am on the fence on is if it makes sense to include this change. If you look at our contribution guide, one of the things we consider when adding functions is the complexity of the function.

This function is a really thin wrapper around the azure SDK. Normally we wouldn't accept such a contribution given the amount of overhead for such a thin wrapper (consider the amount of actual code to the test + example + docs in this PR).

On the other hand, I find it very important to build up a library functions when we are first getting started. For example, it was quite useful from a building confidence perspective to have a good chunk of functions simple (see the ListPods function in k8s for example) or not when we first started with k8s and gcp, as it provided a sense of trust that we supported those clouds. In that regard, I think it would be useful to accept these additions for azure as they come.

Given that, I think I am leaning towards accepting this (after you've addressed my comments below), but we should start to think about these factors once we have a good number of azure functions in terratest.

.terraform-version Outdated Show resolved Hide resolved
modules/azure/actiongroup.go Outdated Show resolved Hide resolved
modules/azure/actiongroup.go Outdated Show resolved Hide resolved
modules/azure/actiongroup.go Show resolved Hide resolved
test/azure/terraform_azure_actiongroup_example_test.go Outdated Show resolved Hide resolved
@tsalright
Copy link
Author

I could see grouping this into a more complicated example like building an entire web app with app service plan, web app, auto scaling, and alerts. That way one example can cover the validation of several modules I would like to add.

The one comment I would bring up is that the Azure SDK for Go is pretty painful to deal with and figure out what to use and I see one of the thinks I like about this is to have consistency with my testing. Terratest already has some core functions that are privately accessible. In my tests I would prefer to access all Azure objects through Terratest's azure.ActionGroup or azure.AppServicePlan or azure.{ResourceName}. Otherwise it would end up with a mix of using some built in Terratest accessors and custom Azure SDK for Go. This is exactly what I did for my modules I created while I have been waiting to get these contributions added to Terratest. See my Azure ActionGroup module for an example of what I would prefer not to do and just consume Terratest.

Variable name cleanup, remove tfenv file, updated comments in module and test
Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

The updates LGTM! Had two minor style nits that I somehow missed on the first go around, but they are not substantial.

I'll hold off on merging this until we resolve the thin wrapper question, but other than that should be good to go!

yorinasub17
yorinasub17 previously approved these changes Sep 18, 2020
Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

Since the thread is resolved, I'll go ahead and approve this. Do you have logs for an updated build run after your changes that you can share?

@tsalright
Copy link
Author

Let me address the last two comments you had this morning and I will get you the build link to show it passed.

James Jackson added 2 commits September 22, 2020 14:09
Comment blocks added to variables and block comment length fixed.
@tsalright
Copy link
Author

I ran the unit tests locally and they passed. Here is a screen shot of my execution with the latest code I just pushed.
image

Here is the execution of the pipeline for the integration tests. https://github.com/tsalright/terratest/runs/1151762422?check_suite_focus=true

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working through the changes with me!

@yorinasub17 yorinasub17 merged commit 472928e into gruntwork-io:master Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants