SUMO-248024: #691

namangoya · 2024-09-20T14:40:00Z

What are we doing?

Adding enabled to the documentation of sumologic_data_forwarding_destination, and adjusted default values.
Adding index_id in sumologic_scheduled_views, and adjusted necessary documentations.

Why is this required?

As a part of supporting Data Forwarding Rules creation via TF for Scheduled Views we need index_id, because the id is coming from the autoview table.

Please review commit by commit.

Testing

Manual testing has been performed via main.tf and we are successfully able to configure a DF Rule for a SV. Attached test results and the test file in the comment.

…_destination, and adjusted default values

…d updated the corresponding documentation

…warding_rule and data_forwarding_destination and added example to correctly configure the data forwarding rule for a scheduled view

…lable after the SV creation

namangoya · 2024-09-23T04:38:48Z

Manual Testing has been performed - Success

Steps to test the main.tf file are mentioned here.
File for manual testing (rename it to main.tf) - main.txt
All 3 resources will be created

raghavxk · 2024-09-23T12:02:29Z

sumologic/sumologic_data_forwarding_rule.go

@@ -62,7 +62,7 @@ func (s *Client) DeleteDataForwardingRule(indexId string) error {
 type DataForwardingRule struct {
 	IndexId       string `json:"indexId"`
 	DestinationId string `json:"destinationId"`
-	Enabled       bool   `json:"enabled,omitempty"`
+	Enabled       bool   `json:"enabled"`


Why are we making this change? This will send value as false instead of not serialising it in json response(which eventually gets treated as null)

The bad part is that enabled = false is treated as null.... and whenever we are trying to set false, it is not sent in the API call...

We are not setting any default for enabled .. so it will not be send as false in case left blank (see here).

I've tested this scenario..

raghavxk · 2024-09-23T12:06:57Z

website/docs/r/data_forwarding_rule.html.markdown

+resource "sumologic_data_forwarding_rule" "test_rule_sv" {
+  index_id       = sumologic_scheduled_view.failed_connections.index_id
+  destination_id = sumologic_data_forwarding_destination.test_destination.id


What happens when user supplies both IndexName and IndexId ?

IndexId will simply be ignored ... as IndexId is never taken into consideration in PUT/POST calls .. please check the API documentation here

We are just using IndexId for creating the DF Rule... it is never updated in the PUT call, nor it is ever required to maintain the SV resource's state.

IndexId will simply be ignored .

This behavior should be documented in the terraform docs.

Yes, we are documenting it in the SV page itself as it belongs to SV.

Updated the docs in the latest commit.

raghavxk · 2024-09-23T12:11:02Z

website/docs/r/data_forwarding_rule.html.markdown

 - `destination_id` - (Required) The data forwarding destination id.
- `enabled` - (Required) True when the data forwarding rule is enabled.
+- `enabled` - (Optional) True when the data forwarding rule is enabled. Will be treated as _false_ if left blank.


Should we add possible values for file_format here to aid customers?

Not needed, we already have a descriptive documentation ... and that is added on top of this page - see here

Not needed

I disagree, we should cover the possible values here. Can't rely on an example to do that.

Shall I update this Rule's TF documentation and add a pointer to the original documentation here (point 6) in that case?

Possible values for file_format are a combination of few placeholders, and adding all of them here will not look good in the TF docs imho.

Updated the docs in the latest commit.

ashpaliwal · 2024-09-23T15:50:05Z

CHANGELOG.md

@@ -2,6 +2,11 @@
 DEPRECATIONS:
 * resource_sumologic_ingest_budget : Deprecated in favour of `resource_sumologic_ingest_budget_v2`.

+## 2.31.5 (September 23, 2024)


Please remove date. Date shall be added on the day of release

raghavxk

lgtm

…m the release

dagould · 2024-09-24T06:37:31Z

CHANGELOG.md

@@ -2,6 +2,11 @@
 DEPRECATIONS:
 * resource_sumologic_ingest_budget : Deprecated in favour of `resource_sumologic_ingest_budget_v2`.



It seems a little strange to be doing a 2.x release while having the 3.0.0 section in the CHANGELOG, with content in it already. But I think it's ok.

(Also, an (Unreleased) on the 2.31.5 line would be appropriate, but there's no need to change it; one of us will be editing that line with the correct date anyway.)

Tbh, I thought that 3.0.0 is written for some other purpose.. and didn't thought of using that, just continued after 2.31.3 ..

@dagould are you planning to maintain two versions of the provider? I would be in favor of just continuing 3.x.x version.

sumovishal · 2024-09-24T16:51:32Z

sumologic/resource_sumologic_data_forwarding_rule.go

@@ -33,12 +33,10 @@ func resourceSumologicDataForwardingRule() *schema.Resource {
 			"enabled": {
 				Type:     schema.TypeBool,
 				Optional: true,
-				Default:  true,


This looks like a change in behavior. A rule will be disabled by default now? This might be a breaking change. The good part is we are scheduled for a major release if this is a breaking change it should be fine. We should document the new behavior.

Amm, no it's not a breaking change. Because in the API, we don't enforce any default value for enabled, which means that it'll be treated as false if left blank.

Here, I'm correcting a mistake from my previous PR (#688) .. and and bringing it in-line with the API.

sumovishal · 2024-09-24T16:55:24Z

website/docs/r/scheduled_view.html.markdown

@@ -23,6 +23,7 @@ QUERY
  retention_period = 365
  lifecycle {
    prevent_destroy = true
+    ignore_changes = [index_id]


We should add a comment here explaining why we need to add index_id to ignore_changes.

I've added in this same doc a little below at line 48.

sumovishal · 2024-09-24T17:00:33Z

website/docs/r/scheduled_view.html.markdown

@@ -44,6 +45,7 @@ The following arguments are supported:
 The following attributes are exported:

 - `id` - The internal ID of the scheduled view.
+- `index_id` - The Index ID of the scheduled view. It never updates at any point of time during resource updates, therefore make sure to ignore this via `ignore_changes = [index_id]`.


This seems like a gap in the API. index_id is a write-only field and it never updates after the initial creation. This behavior should have been enforced in the API rather than relying on users to not update index_id.

This is already enforced in the API (PUT and POST api docs). Here we are just communicating/informing customers as it is a non-updatable field.

Even if the user passes index_id, it will simply be ignored at the API.

Even if the user passes index_id, it will simply be ignored at the API.

I don't think this is right behavior. We are silently dropping a field provided by the client. Instead we should return error b/c request is invalid as you are not allowed to change the index id. This is the real issue here.

It should be noted that ignore_changes is not strictly required if you don't change the index id which I believe most customer won't do, given we don't support it?

It should be noted that ignore_changes is not strictly required if you don't change the index id which I believe most customer won't do, given we don't support it?

Yes, not a strict requirement to add ignore_changes .. but recommended, and hence conveying that msg in the documentation here.

I don't think this is right behavior. We are silently dropping a field provided by the client. Instead we should return error b/c request is invalid as you are not allowed to change the index id. This is the real issue here.

This will not even reach the API layer as it'll be dropped by OpenAPI translation itself and will not be translated into the request payload's Java Object.

Moreover, there can be some invalid fields added by the user (by mistake or intentionally), so those are also simply ignored and not translated by OpenAPI...

Therefore, we can't really do much with this index_id field being ignored.

This will not even reach the API layer as it'll be dropped by OpenAPI translation itself and will not translated into the request payload's Java Object.

We can follow-up internally on this one. I don't see a reason why OpenAPI would silently drop a field.

Yes, not a strict requirement to add ignore_changes .. but recommended, and hence conveying that msg in the documentation here.

The language should be updated to reflect that. Currently, it seems we are asking to add ignore_changes rather than a recommendation.

We can follow-up internally on this one. I don't see a reason why OpenAPI would silently drop a field.

Sure, can I know the set of people who can help me here?

Also, is it a blocker for this PR?

The language should be updated to reflect that. Currently, it seems we are asking to add ignore_changes rather than a recommendation.

But without ignore_changes, we are running into this issue.. let's continue for this on the slack thread.

But without ignore_changes, we are running into this issue.. let's continue for this on the slack thread.

Issue is resolved with the introduction of computed prop for index_id. (see thread)

I'm updating the documentation that ignore_changes is just a recommendation and not a mandatory thing.

…mentations as per the review comments.

sumovishal · 2024-09-26T02:51:01Z

website/docs/r/scheduled_view.html.markdown

@@ -44,6 +45,7 @@ The following arguments are supported:
 The following attributes are exported:

 - `id` - The internal ID of the scheduled view.
+- `index_id` - The Index ID of the scheduled view. It remains unchanged during resource updates, and any manual modifications will be disregarded. While it’s not mandatory, we recommend to ignore this via `ignore_changes = [index_id]`.


Now we have made this field as computed you can drop the "modification" and ignore_changes part

Yeah I have just kept if for the fact that if someone explicitly tries to set index_id = "000000ABCDE..." (some random value) in the resource definition, then with the help of ignore_changes that will be ignored.

sumovishal · 2024-09-26T02:51:35Z

website/docs/r/data_forwarding_rule.html.markdown

+  retention_period = 1
+  lifecycle {
+    prevent_destroy = true
+    ignore_changes = [index_id]


This should not needed now?

Yeah I have just kept if for the fact that if someone explicitly tries to set index_id = "000000ABCDE..." (some random value) in the resource definition, then with the help of ignore_changes that will be ignored.

namangoya added 3 commits September 20, 2024 17:59

SUMO-248024: adding to the documentation of sumologic_data_forwarding…

bbd6b34

…_destination, and adjusted default values

SUMO-248048: adding index_id to resource_sumologic_scheduled view, an…

5dab9b8

…d updated the corresponding documentation

SUMO-248048: updated the documentations for scheduled_views, data_for…

78a8578

…warding_rule and data_forwarding_destination and added example to correctly configure the data forwarding rule for a scheduled view

namangoya force-pushed the adding_index_Id_in_scheduled_view branch from fdf9bc4 to 78a8578 Compare September 21, 2024 05:10

namangoya added 2 commits September 21, 2024 11:00

SUMO-248048: added changes in the CHANGELOG.md

723064f

SUMO-248048: making index_id as a computed field, which makes it avai…

138fe92

…lable after the SV creation

namangoya requested a review from ssharma-sumologic September 23, 2024 07:57

namangoya marked this pull request as ready for review September 23, 2024 07:57

namangoya requested review from sumovishal and dagould as code owners September 23, 2024 07:57

raghavxk reviewed Sep 23, 2024

View reviewed changes

namangoya requested a review from raghavxk September 23, 2024 13:27

ashpaliwal reviewed Sep 23, 2024

View reviewed changes

raghavxk approved these changes Sep 23, 2024

View reviewed changes

SUMO-248048: addressed review comment by Ashish, removed the date fro…

0789980

…m the release

namangoya requested a review from ashpaliwal September 23, 2024 17:21

SUMO-248048: fixed the data_forwarding_destination's documentation

d33dd93

namangoya force-pushed the adding_index_Id_in_scheduled_view branch from 70b1648 to d33dd93 Compare September 24, 2024 04:13

dagould reviewed Sep 24, 2024

View reviewed changes

namangoya requested review from dagould and sjain05 September 24, 2024 15:44

sumovishal reviewed Sep 24, 2024

View reviewed changes

SUMO-248048: updated scheduled_view's and data_forwarding_rule's docu…

651ae55

…mentations as per the review comments.

sumovishal reviewed Sep 26, 2024

View reviewed changes

sjain05 approved these changes Sep 26, 2024

View reviewed changes

namangoya merged commit ec0fcb0 into master Sep 26, 2024
7 checks passed

namangoya deleted the adding_index_Id_in_scheduled_view branch September 26, 2024 15:27

		@@ -2,6 +2,11 @@
		DEPRECATIONS:
		* resource_sumologic_ingest_budget : Deprecated in favour of `resource_sumologic_ingest_budget_v2`.

SUMO-248024: #691

SUMO-248024: #691

Conversation

namangoya commented Sep 20, 2024 • edited Loading

What are we doing?

Why is this required?

Please review commit by commit.

Testing

namangoya commented Sep 23, 2024 • edited Loading

Manual Testing has been performed - Success

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

namangoya Sep 23, 2024 • edited Loading

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

sumovishal Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

namangoya Sep 25, 2024 • edited Loading

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

raghavxk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namangoya Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namangoya Sep 24, 2024 • edited Loading

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

namangoya Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namangoya Sep 24, 2024 • edited Loading

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

namangoya Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

namangoya commented Sep 20, 2024 •

edited

Loading

namangoya commented Sep 23, 2024 •

edited

Loading

namangoya Sep 23, 2024 •

edited

Loading

sumovishal Sep 24, 2024 •

edited

Loading

namangoya Sep 25, 2024 •

edited

Loading

namangoya Sep 24, 2024 •

edited

Loading

namangoya Sep 24, 2024 •

edited

Loading

namangoya Sep 24, 2024 •

edited

Loading

namangoya Sep 24, 2024 •

edited

Loading

namangoya Sep 26, 2024 •

edited

Loading