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

azurerm_sentinel_watchlist - Add required property item_search_key #15861

Merged
merged 3 commits into from
Apr 14, 2022

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Mar 17, 2022

Since March, the sentinel API now requires the itemSearchey in the PUT request. Meanwhile, the contentType is now not needed here as we don't actually send any content, but delegate it to the azurerm_sentinel_watchlist_item.

Fixes #15851.
Superseds: #15856.

Test

💤  TF_ACC=1 go test -v -timeout=20h ./internal/services/sentinel  -run='TestAccWatchlist'
=== RUN   TestAccWatchlistItem_basic
=== PAUSE TestAccWatchlistItem_basic
=== RUN   TestAccWatchlistItem_update
=== PAUSE TestAccWatchlistItem_update
=== RUN   TestAccWatchlistItem_requiresImport
=== PAUSE TestAccWatchlistItem_requiresImport
=== RUN   TestAccWatchlist_basic
=== PAUSE TestAccWatchlist_basic
=== RUN   TestAccWatchlist_complete
=== PAUSE TestAccWatchlist_complete
=== RUN   TestAccWatchlist_requiresImport
=== PAUSE TestAccWatchlist_requiresImport
=== CONT  TestAccWatchlistItem_basic
=== CONT  TestAccWatchlist_requiresImport
=== CONT  TestAccWatchlist_basic
=== CONT  TestAccWatchlistItem_requiresImport
=== CONT  TestAccWatchlist_complete
=== CONT  TestAccWatchlistItem_update
--- PASS: TestAccWatchlist_basic (124.25s)
--- PASS: TestAccWatchlistItem_requiresImport (150.89s)
--- PASS: TestAccWatchlistItem_basic (184.24s)
--- PASS: TestAccWatchlistItem_update (228.30s)
--- PASS: TestAccWatchlist_requiresImport (784.88s)
--- PASS: TestAccWatchlist_complete (820.07s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/sentinel      820.092s

Since March, the sentinel API now requires the `itemSearchey` in the `PUT` request. Meanwhile, the `contentType` is now not needed here as we don't actually send any content, but delegate it to the `azurerm_sentinel_watchlist_item`.
@magodo
Copy link
Collaborator Author

magodo commented Mar 17, 2022

The CI failure of the example is due to there is new property introduced.

@mcamenzind
Copy link

Any reason for deviating from Microsoft's property naming itemsSearchKey (notice the s)?

// Setting them here is merely to make the API happy.
Source: securityinsight.Source("a.csv"),
ContentType: utils.String("Text/Csv"),
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a breaking change, we should be exposing a field for this with a default value here?

// Setting them here is merely to make the API happy.
Source: securityinsight.Source("a.csv"),
ContentType: utils.String("Text/Csv"),
Source: securityinsight.Source("a.csv"),
Copy link
Contributor

Choose a reason for hiding this comment

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

the possible values from the SDK here are:

// Source enumerates the values for source.
type Source string

const (
	// SourceLocalfile ...
	SourceLocalfile Source = "Local file"
	// SourceRemotestorage ...
	SourceRemotestorage Source = "Remote storage"
)

// PossibleSourceValues returns an array of possible values for the Source const type.
func PossibleSourceValues() []Source {
	return []Source{SourceLocalfile, SourceRemotestorage}
}

why is this a.csv?

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 @magodo - left some comments inline. as for the the name items_seach_key sounds incorrect? correct grammar should be item_search_key regardless of what the SDK is using?

// Setting them here is merely to make the API happy.
Source: securityinsight.Source("a.csv"),
ContentType: utils.String("Text/Csv"),
Source: securityinsight.Source("a.csv"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

"a.csv" is not a valid value for securityinsight.Source? at that should we not expose this in schema with a default?

// Setting them here is merely to make the API happy.
Source: securityinsight.Source("a.csv"),
ContentType: utils.String("Text/Csv"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

how come we are removing this? should it not be in schema with a default?

ContentType: utils.String("Text/Csv"),
Source: securityinsight.Source("a.csv"),

ItemsSearchKey: utils.String(model.ItemsSearchKey),
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto here? should this not just be in schema with a default?

This reverts commit e7cfdf0.
@magodo
Copy link
Collaborator Author

magodo commented Mar 23, 2022

@tombuildsstuff and @katbyte The Source and ContentType are not returned from API, they are used by the service during the PUT so that the service can create watchlist items based on the specified source. However, as is stated in #14258 this resource is only to create an empty watchlist, if users want to create watchlist items, they can then use azurerm_sentinel_watchlist_item resource.

Therefore, this PR removes the ContentType (as is actually optional in turns of API), and the value of the Source is actually doesn't have any real effect, but only to make API happy, which even can be regarded as not a breaking change for TF users.

About the naming of the items_search_key, I've reverted my last commit.

@Andyandpandy
Copy link

Any progress here?
This PR is blocking me from going > 3.0.x

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 🚜

@katbyte katbyte merged commit df791da into hashicorp:main Apr 14, 2022
@github-actions github-actions bot added this to the v3.2.0 milestone Apr 14, 2022
katbyte added a commit that referenced this pull request Apr 14, 2022
@github-actions
Copy link

This functionality has been released in v3.2.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2022
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.

properties not properly set in azurerm_sentinel_watchlist (missing itemsSearchKey, rawContent)
5 participants