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

New Resource - azurerm_data_factory_linked_service_blob_[blob|storage|sfpt] #6366

Merged
merged 30 commits into from Jul 8, 2020
Merged

Conversation

markti
Copy link
Contributor

@markti markti commented Apr 6, 2020

Feature #4029
Feature #6365
Feature #6167
Feature #6413
Feature #6166

First time writing in golang, ever. First time contributor. Please be nice to me. :o)

W.r.t. the Web Linked service. I did not implement Basic / ClientCredential. Is that ok? Assuming I get everything working would it be included in the provider without all authentication types supported?

I have a problem with the acceptance tests. They fail on the data.ImportStep(). I was unable to test these resources because I'm unfamiliar with how to test a terraform provider plugin. When I run the acceptance tests, I see new resource groups / data factories provisioned in my Azure Subscription. I assume this means that the Data Factory / Linked Service deployed properly but maybe I am not loading the API response back into state properly. Any idea why I am getting these errors in Acceptance test?

A couple questions from a newbie:

  1. is there a way to pause the acceptance tests so I can go manually inspect what was provisioned in Azure?
  2. Alternatively, What's the best way to see the REST API calls that Terraform is making?

Thank you so much in advance!

Mark

(fixes #4029 fixes #6365 fixes #6167 fixes #6413 fixes #6166)

@katbyte katbyte changed the title web and sftp data factory resources New Resource - azurerm_data_factory_linked_service_blob_[blob|storage|sfpt] Apr 7, 2020
Copy link
Collaborator

@katbyte katbyte 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 new resources @markti!

Over all these look great and i've left some minor comments inline. In addition to these could we add documentation for the new resource? thanks!

}

resource "azurerm_data_factory_linked_service_blob_storage" "test" {
name = "acctestlssql%d"
Copy link
Collaborator

Choose a reason for hiding this comment

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

For any property that accepts capitals could we use some to ensure the api handles them correctly?

}

resource "azurerm_data_factory" "test" {
name = "acctestdf%d"
Copy link
Collaborator

Choose a reason for hiding this comment

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

For any property that accepts capitals could we use some to ensure the api handles them correctly?

@katbyte katbyte added this to the v2.5.0 milestone Apr 7, 2020
@markti
Copy link
Contributor Author

markti commented Apr 7, 2020

Thanks for the new resources @markti!

Over all these look great and i've left some minor comments inline. In addition to these could we add documentation for the new resource? thanks!

Thanks!
I’ll have to look at the readme about generating documentation. Any idea why I didn’t pass terrfmt? I had a question regarding authentication type if I could pick your brain there it would be helpful maybe I can include all three auth types. My golang is very fresh.

@ghost ghost removed the waiting-response label Apr 7, 2020
@katbyte
Copy link
Collaborator

katbyte commented Apr 7, 2020

@markti, documentation generation is still fairly new and most resources are written without it. You should be able to just copy some of the existing documentation and only have to make minor changes.

For terrafmt it could be a bunch of things, but you can just run it on the files listed in the output (or the entire provider) and it'll automatically fix it 🙂

And of course, i'm not sure what you mean by the authentication type? the parse function & id type? (also feel free to reach out to me on our community slack if you'd like - link in our readme)

@tombuildsstuff tombuildsstuff modified the milestones: v2.5.0, v2.6.0 Apr 8, 2020
@tombuildsstuff tombuildsstuff removed this from the v2.6.0 milestone Apr 16, 2020
@markti
Copy link
Contributor Author

markti commented Apr 17, 2020

@katbyte sorry I made some updates to the code, some renaming of files which caused some of your feedback to become outdated.

@ghost ghost removed the waiting-response label Apr 17, 2020
Copy link
Contributor Author

@markti markti left a comment

Choose a reason for hiding this comment

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

@markti, documentation generation is still fairly new and most resources are written without it. You should be able to just copy some of the existing documentation and only have to make minor changes.

For terrafmt it could be a bunch of things, but you can just run it on the files listed in the output (or the entire provider) and it'll automatically fix it 🙂

And of course, i'm not sure what you mean by the authentication type? the parse function & id type? (also feel free to reach out to me on our community slack if you'd like - link in our readme)

@katbyte I'm running terrafmt on the files

"terrafmt -f -v ./path/to/file.go" but it is telling me that "0/3 blocks need formatting"

@markti markti requested a review from katbyte April 17, 2020 15:41
@katbyte katbyte added this to the v2.7.0 milestone Apr 21, 2020
@katbyte katbyte modified the milestones: v2.11.0, v2.12.0, v2.13.0 May 22, 2020
@tombuildsstuff tombuildsstuff modified the milestones: v2.13.0, v2.14.0, v2.15.0 Jun 4, 2020
@katbyte katbyte modified the milestones: v2.15.0, v2.16.0 Jun 18, 2020
@tombuildsstuff tombuildsstuff modified the milestones: v2.16.0, v2.17.0 Jun 25, 2020
@katbyte katbyte modified the milestones: v2.17.0, v2.18.0 Jul 3, 2020
Copy link
Collaborator

@katbyte katbyte 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 those changes! aside from some files being renamed (which i will do now) this ltgm now 🙂

@katbyte katbyte merged commit 823d27c into hashicorp:master Jul 8, 2020
katbyte added a commit that referenced this pull request Jul 8, 2020
@ghost
Copy link

ghost commented Jul 10, 2020

This has been released in version 2.18.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.18.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Aug 7, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked and limited conversation to collaborators Aug 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.