Add data_source_arm_scheduled_time data source and tests #353

Kemyke · 2017-09-25T13:27:09Z

Data source for scheduling time in Azure. Suggested by @tombuildsstuff in the PR257

tombuildsstuff · 2017-10-02T16:23:42Z

azurerm/data_source_arm_scheduled_time.go

+
+	var firstRunSec, firstRunMinute, firstRunHour, firstRunDayOfWeek, firstRunDayOfMonth int
+
+	//TODO: GetOk is not suitable here because it returns ok=false if the second value is 0. The GetOkExists looks good but it hasn't merged yet


I'll send a PR to update the Vendored Terraform version to 0.10.6 to be able to make use of this

@Kemyke heads up that this PR has now merged, so once this is rebased we should be able to update this to use GetOkExists :)

Hi @tombuildsstuff
sorry for the delayed answer, now i have time to complete this PR.
Right now i am having a problem with this updated code. I can't build the code, the build fails with the error: vendor/github.com/hashicorp/terraform/config/testing.go:9: t.Helper undefined (type *testing.T has no field or method Helper).

So i am blocked now, do you have any idea what am i doing wrong?

I just did a clean clone and now everything is working so i changed the code to use GetOkExists.

tombuildsstuff

Hey @Kemyke

Thanks for submitting this PR - apologies for the delay in reviewing!

I've taken a look through and this is off to a good start - there's a couple of things worth mentioning:

I've updated the vendored Terraform version to 0.10.6, as such we should be able to make use of GetOkExists
could we add some additional acceptance tests covering each of the Frequency values?

Thanks!

tombuildsstuff · 2017-10-04T11:36:42Z

azurerm/data_source_arm_scheduled_time.go

+
+	var firstRunSec, firstRunMinute, firstRunHour, firstRunDayOfWeek, firstRunDayOfMonth int
+
+	//TODO: GetOk is not suitable here because it returns ok=false if the second value is 0. The GetOkExists looks good but it hasn't merged yet


@Kemyke heads up that this PR has now merged, so once this is rebased we should be able to update this to use GetOkExists :)

tombuildsstuff · 2017-10-04T11:42:07Z

azurerm/data_source_arm_scheduled_time.go

+			},
+			"timezone": {
+				Type:     schema.TypeString,
+				Optional: true,


I think this also wants to be Computed?

tombuildsstuff · 2017-10-04T11:47:38Z

azurerm/data_source_arm_scheduled_time.go

+			"day_of_month": {
+				Type:     schema.TypeInt,
+				Optional: true,
+				Default:  -1,


I might be wrong, but I think this wants a ConflictsWith: []string { "day_of_week" } so it can't be used when Day of Week is set (and vica-versa)?

tombuildsstuff · 2017-10-04T13:58:37Z

azurerm/data_source_arm_scheduled_time_test.go

+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(dataSourceName, "hour", strconv.Itoa(now.Hour()+1)),
+					resource.TestCheckResourceAttr(dataSourceName, "minute", "0"),
+					resource.TestCheckResourceAttr(dataSourceName, "second", "0"),


it'd be good for one of these tests to set the Second argument if possible?

tombuildsstuff · 2017-10-04T14:00:24Z

azurerm/data_source_arm_scheduled_time.go

+					string(Hour),
+					string(Month),
+					string(OneTime),
+					string(Week),


it'd be good to add an acceptance test for each of these if possible

also: does OneTime make sense in this context (as a data source) or would you be more likely to know that and specify the time period? 🤔

I added tests for all the possible periodicity. I can imagine a case when the OneTime make sense, so i would leave it there if you don't mind.

tombuildsstuff · 2017-10-04T14:01:18Z

azurerm/data_source_arm_scheduled_time.go

+	Week    Frequency = "Week"
+)
+
+func dataSourceArmScheduledTime() *schema.Resource {


could we add some documentation for this data source? the relevant files are ./website/d/scheduled_time.html.markdown and ./website/azurerm.erb

…exogen-international/terraform-provider-azurerm into data_source_arm_scheduled_time

Kemyke · 2017-10-25T11:07:46Z

Hi @tombuildsstuff

i think i finished with the fixes of all the comments. Please take a look at the modifications.

Thanks!

Kemyke · 2017-11-28T13:31:27Z

Hi @tombuildsstuff
May i ask why are we stucked? Is there a problem with the PR or you don't have the time to merge?

Thanks!

tombuildsstuff · 2017-12-04T21:06:08Z

hey @Kemyke

Apologies for the delayed response here!

Unfortunately this has been delayed due to a lack of time on our front - with holidays and then some other tasks taking priority - this has led to a bit of a backlog which we're still trying to work through. Sorry about this - I'm taking a look into this now :)

Thanks!

tombuildsstuff

Hey @Kemyke

Thanks for pushing those updates - apologies for the delayed re-review here!

I've taken another look through and left some comments in-line - mostly we just need to ensure the tests match the schema (in that, we test Optional fields both when they're specified and unset) - but this is generally looking good :)

Thanks!

tombuildsstuff · 2017-12-04T12:38:08Z

website/docs/d/schduled_time.html.markdown

+## Attributes Reference
+
+The following attributes are exported:
+* `next_run_time` - The value when should this schedule next run. This is computed from the previous parameters.


The value when should this schedule next run. This is computed from the previous parameters.

I'd suggest this would make more sense as The RFC3339 formatted date when the schedule should next run

tombuildsstuff · 2017-12-04T12:38:42Z

website/docs/d/schduled_time.html.markdown

+* `minute` - (Optional) Specifies the minute part of the schedule time.
+* `hour` - (Optional) Specifies the hour part of the schedule time.
+* `day_of_week` - (Optional) Specifies the day of the week when the schedule time will be triggered. Conflicts with `day_of_week`.
+* `day_of_month` - (Optional) Specifies the day of the month when the schedule time will be triggered. Conflicts with `day_of_month`.


Conflicts with day_of_month.

This should be "Conflicts with day_of_week."

tombuildsstuff · 2017-12-04T12:38:50Z

website/docs/d/schduled_time.html.markdown

+* `second` - (Optional) Specifies the second part of the schedule time.
+* `minute` - (Optional) Specifies the minute part of the schedule time.
+* `hour` - (Optional) Specifies the hour part of the schedule time.
+* `day_of_week` - (Optional) Specifies the day of the week when the schedule time will be triggered. Conflicts with `day_of_week`.


Conflicts with day_of_week.

This should be "Conflicts with day_of_month."

tombuildsstuff · 2017-12-04T12:39:23Z

website/docs/d/schduled_time.html.markdown

+  Get information about the specified scheduled time.
+---
+
+# azurerm\_scheduled\_time


minor we actually don't need these backslashes

tombuildsstuff · 2017-12-04T12:50:54Z

azurerm/data_source_arm_scheduled_time.go

+			"timezone": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Computed: true,


can we add a ValidateFunc for this to ensure that it's valid and caught at plan time? This can be done as follows:

Default: time.UTC.String(), ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) { locationString := v.(string) if _, err := time.LoadLocation(locationString); err != nil { errors = append(errors, fmt.Errorf("Cannot parse timezone: %q", locationString)) } return },

In addition - I believe we should be able to default the TimeZone to UTC in the Schema - as shown above (which means we can remove the if statement below)

@tombuildsstuff Default must be null if the field is computed!

tombuildsstuff · 2017-12-04T13:14:48Z

azurerm/data_source_arm_scheduled_time_test.go

+        "day_of_month" = "%d"
+        "hour" = "%d"
+        "minute" = "0"
+        "second" = "0"


given these fields are optional - it'd be good to add tests covering the hour, minute and second fields being unspecified (as right now all of these fields are specified in each test)

Yepp, i was thinging about it. I wrote a test but it had problems with asserting.
If i don't specify the hour, minutes and seconds it will be evaluated from the current time. So the test can't check the next_run_time field because it is computed during the tests. I don't have any idea how to avoid this. Is it make sense to write the test without asserting the next_run_time? Or do you have any other idea?

It was also a pain to do the assertion for these tests. I am not really satisfied with them. I am wondering to rewrite some of the tests and use conrete times instead of this relaitve ones (eg. now.Add(time.Duration(-1) * time.Minute)) Right now i am not 100% percent sure that these test will always pass, maybe the outcome is time dependent.

I fixed some assertion and remove one test which would be fail sometimes (i described the scenario under your comment)

Added a test with partially specified schema.

tombuildsstuff · 2017-12-04T21:11:00Z

azurerm/data_source_arm_scheduled_time.go

+		shiftTime = v.(int)
+	} else {
+		shiftTime = 0
+	}


we shouldn't need the else here - since it's an Integer it'll be 0 by default (and exists will always be true)

tombuildsstuff · 2017-12-04T21:14:12Z

website/docs/d/schduled_time.html.markdown

+        "frequency" = "Day"
+        "hour" = "14"
+        "minute" = "25"
+        "minimum_delay_from_now_in_minutes" = "6"


we don't need the quotes around the key part of these values, or where they're Int's - so I think we can make this:

frequency = "Day" hour = 14 minute = 25

tombuildsstuff · 2017-12-04T21:27:53Z

azurerm/data_source_arm_scheduled_time_test.go

+	dataSourceName := "data.azurerm_scheduled_time.test"
+
+	now := time.Now().UTC()
+	scheduletime := now.Add(-25 * time.Duration(24) * time.Hour) //25 days


I think it'd be better to construct a date as below - specifying now.Month() - 1?

Yeah, that was my first attempt but it won't work, if i am right. Can you validate it pls?
If its the end of the month the test will fail. Eg. at the end of march the now is 2017-03-31 if the scheduled date is constructed with month() - 1 it will become 2017-02-31 which is in golang the same as 2017-03-03 so the assertion will fail because we expect March but will get April.

I don't have any idea to workaround this so right now i removed this test.

tombuildsstuff · 2017-12-04T21:28:30Z

azurerm/data_source_arm_scheduled_time_test.go

+	dataSourceName := "data.azurerm_scheduled_time.test"
+
+	now := time.Now().UTC()
+	scheduletime := now.Add(25 * time.Duration(24) * time.Hour) //25 days


(as below) I think it'd be better to construct the date and add a month here - that would remove the need for the if statement?

Kemyke · 2017-12-20T15:48:08Z

@tombuildsstuff I think i am done with more or less everything you asked. I have some questions and dilemmas regarding the tests. Can you please verify my fixes and discuss that questions?
Thanks!

katbyte · 2018-10-24T21:53:31Z

Hi,

Thank you for the PR, however I don't think it is a good fit for the provider at this time. I am unsure of how this would fit into the current automation resources as scheduled time allows for a start_time to easily be set via a input variable.

Feel free to reach out to me if I'm wrong, I would be happy to discuss.

ghost · 2019-03-06T14:18:04Z

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!

root and others added 3 commits September 25, 2017 13:23

Add data_source_arm_scheduled_time data source and tests

49ed8ea

Merge branch 'master' into data_source_arm_scheduled_time

f695f65

Fix formatting

5ff6ee4

tombuildsstuff added enhancement new-data-source labels Oct 2, 2017

tombuildsstuff reviewed Oct 2, 2017

View reviewed changes

tombuildsstuff reviewed Oct 4, 2017

View reviewed changes

tombuildsstuff added the waiting-response label Oct 4, 2017

Kemyke added 3 commits October 7, 2017 00:49

Merge branch 'master' into data_source_arm_scheduled_time

d329143

Fix formatting

a1ea48c

Use GetOkExists

29bff25

tombuildsstuff removed the waiting-response label Oct 11, 2017

Kemyke and others added 10 commits October 12, 2017 12:52

Merge branch 'master' into data_source_arm_scheduled_time

5dfd749

Add ConflictsWit and Computed flags

8e3be3d

Add test with seconds

a8f39fe

Merge branch 'master' into data_source_arm_scheduled_time

5b78363

Add documentation

9747340

Merge branch 'data_source_arm_scheduled_time' of https://github.com/n…

97f90df

…exogen-international/terraform-provider-azurerm into data_source_arm_scheduled_time

Add weekly tests and fix

ce9d6d7

Add monthly tests

f9314c2

Fix formatting

d75bdfb

Add OneTime test

8a89f4c

tombuildsstuff suggested changes Dec 4, 2017

View reviewed changes

tombuildsstuff added the waiting-response label Dec 4, 2017

Attila Kemeny added 3 commits December 19, 2017 10:32

Remove unnecessary else

6431302

Add Computed flags

b80e36a

Add default timezone, validate timezone schema

7f31ccb

Attila Kemeny added 5 commits December 19, 2017 11:59

Remove unnecessary validation

23fcff6

Fix documentation

81f7a1e

Remove computed tag, add location from timezone

43689e9

Refactor tests

b71ef5b

Add partially defined test

bfe4ddf

metacpp self-requested a review August 2, 2018 23:38

metacpp self-assigned this Aug 2, 2018

katbyte closed this Oct 24, 2018

ghost locked and limited conversation to collaborators Mar 6, 2019

ghost removed the waiting-response label Mar 6, 2019

metacpp removed their assignment Mar 6, 2019


		var firstRunSec, firstRunMinute, firstRunHour, firstRunDayOfWeek, firstRunDayOfMonth int

		//TODO: GetOk is not suitable here because it returns ok=false if the second value is 0. The GetOkExists looks good but it hasn't merged yet

Add data_source_arm_scheduled_time data source and tests #353

Add data_source_arm_scheduled_time data source and tests #353

Conversation

Kemyke commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kemyke Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kemyke commented Oct 25, 2017

Kemyke commented Nov 28, 2017

tombuildsstuff commented Dec 4, 2017

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kemyke commented Dec 20, 2017

katbyte commented Oct 24, 2018

ghost commented Mar 6, 2019

Kemyke Oct 9, 2017 •

edited

Loading