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_trigger_schedule #4793

Merged
merged 6 commits into from
Nov 13, 2019

Conversation

mbfrahry
Copy link
Member

@mbfrahry mbfrahry commented Nov 1, 2019

--- PASS: TestAccAzureRMDataFactoryTriggerSchedule_basic (84.33s)
--- PASS: TestAccAzureRMDataFactoryTriggerSchedule_complete (103.15s)

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 @mbfrahry, aside from a few minor comments this LGTM

Comment on lines 5 to 13
"github.com/Azure/go-autorest/autorest/date"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/suppress"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/validate"
"log"
"regexp"
"time"

"github.com/Azure/azure-sdk-for-go/services/datafactory/mgmt/2018-06-01/datafactory"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we run goimports on this file?

},

// There's a bug in the Azure API where this is returned in lower-case
// BUG: https://github.com/Azure/azure-rest-api-specs/issues/5788
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest reopening this on the sdk as they seem to actually act on issues there... sometimes 😪

azurerm/resource_arm_data_factory_trigger_schedule.go Outdated Show resolved Hide resolved
azurerm/resource_arm_data_factory_trigger_schedule.go Outdated Show resolved Hide resolved
Optional: true,
Default: "UTC",
ValidateFunc: validate.NoEmptyStrings,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a few different timezone validation functions, perhaps one would work here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm hesitant to add any validation here and might actually just remove this value. It doesn't seem to do anything and the UI just defaults the time to UTC anyway

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you pick PST the API still takes the value as UTC?

Then i would definitely remove it, add a note that the time is always UC, and open a bug on the SDK with a link in the code explaining why UTC has been ignored.

Copy link
Member Author

Choose a reason for hiding this comment

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

The Data Factory API attributes tend to accept any value without validation. You can submit anything to timezone and it'll be accepted whether it's valid or not but Pacific Standard Time didn't change the format of the time at all so I've submitted an issue on the sdk as a start. Azure/azure-sdk-for-go#6244

Copy link
Collaborator

Choose a reason for hiding this comment

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

fun

@mbfrahry mbfrahry changed the title New Resource: azurerm_data_factory_trigger_schedule [WIP] New Resource: azurerm_data_factory_trigger_schedule Nov 3, 2019
@mbfrahry
Copy link
Member Author

mbfrahry commented Nov 3, 2019

Review addressed but I need to add one more piece before it's ready

@mbfrahry mbfrahry changed the title [WIP] New Resource: azurerm_data_factory_trigger_schedule New Resource: azurerm_data_factory_trigger_schedule Nov 4, 2019
@mbfrahry
Copy link
Member Author

mbfrahry commented Nov 4, 2019

This should be ready for a final review @katbyte if you think it's necessary

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.

LGTM 👍

Optional: true,
Default: "UTC",
ValidateFunc: validate.NoEmptyStrings,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

fun

@mbfrahry mbfrahry merged commit d36e613 into master Nov 13, 2019
@mbfrahry mbfrahry deleted the f-datafactory-trigger-schedule branch November 13, 2019 19:31
@ghost
Copy link

ghost commented Nov 26, 2019

This has been released in version 1.37.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 = "~> 1.37.0"
}
# ... other configuration ...

@neil-yechenwei
Copy link
Contributor

@mbfrahry , I noticed you made a PR (#3244) to add support for azurerm_data_factory_pipeline. I found you added a property "activity" in your first commit (06bd322). But you deleted this property in later commit (6822cfa). After learned from the azure doc for data factory (https://docs.microsoft.com/en-us/azure/data-factory/v1/data-factory-copy-activity-tutorial-using-powershell), I think you should retain this property "activity" since I think it's an important thing for data factory pipeline to combine the InputDataset and the OutputDataset. May I know why you removed the property "activity"? If this property is necessary, can it be added back?

@ghost
Copy link

ghost commented Feb 17, 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 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Feb 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants