-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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_eventgrid_topic
- support for input_schema
, input_mapping_fields
, and input_mapping_default_values
#6858
azurerm_eventgrid_topic
- support for input_schema
, input_mapping_fields
, and input_mapping_default_values
#6858
Conversation
azurerm_evengrid_topic
: add input schema support
azurerm_evengrid_topic
: add input schema supportazurerm_evengrid_topic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jrauschenbusch, thanks for opening this! I just have a few things that we could do to harden the attributes and prevent some crashes
azurerm/internal/services/eventgrid/eventgrid_topic_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/eventgrid/eventgrid_topic_resource.go
Outdated
Show resolved
Hide resolved
ForceNew: true, | ||
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"id": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we try and force a user to pass in at least one of these attributes? I'm wondering what happens if a user doesn't pass anything into this for whatever reason. We could add
AtLeastOneOf: []string{"input_mapping_files.0.id", "input_mapping_files.0.topic".....}
to achieve this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with that. It would be really nice to give the end user a hint that there might be a misuse, but AtLeastOneOf
conflicts with the Optional
flag. However, it must be optional because the input_mapping is only used when the input_schema is changed to CustomEventSchema
. EventGridSchema
is used by default. In this case, an input_mapping is just ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made some tests now.
When using EventGridSchema
or CloudEventV01Schema
as input_schema, and we add an (empty) input_mapping_fields and/or input_mapping_default_values block, the following error will be reported:
Error: eventgrid.TopicsClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="InvalidRequest" Message="InputSchemaMapping must not be provided when InputSchema is EventGridSchema or CloudEventV01Schema."
When using CustomEventSchema
as input_schema and adding empty input_mapping_fields and input_mapping_default_values blocks the following will be reported:
Error: eventgrid.TopicsClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="InvalidRequest" Message="Invalid InputSchemaMapping: DataVersion entry cannot be null, and either sourceField or defaultValue must be specified."
After adding a sourceField or defaultValue for data_version
the following is reported:
Error: eventgrid.TopicsClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="InvalidRequest" Message="Invalid InputSchemaMapping: Invalid Subject. Either sourceField or defaultValue should be specified."
After adding a sourceField or defaultValue for subject
the following is reported:
Error: eventgrid.TopicsClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="InvalidRequest" Message="Invalid InputSchemaMapping: Invalid EventType. Either sourceField or defaultValue should be specified."
After adding a sourceField or defaultValue for event_type
the resource could finally be created.
Hence at Plan time we would need the following checks:
- if
input_schema != "CustomEventSchema"
, then the blocks input_mapping_fields and input_mapping_default_values must not be present - if
input_schema == "CustomEventSchema"
, thensubject
,event_type
anddata_version
are required in one of the input_mapping(_default) blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I have an idea to make this easier on users and it's to split these attributes out into their own respective lists. InputSchemaCloudEventSchemaV10
and InputSchemaEventGridSchema
can just be booleans that conflict with each other like is_event_grid_schema
and then have custom_event_schema_input_mapping
with all of these variables in there. That will let us better differentiate the Required
and Optional
attributes. While being harder from a maintainer perspective, it should really help users see how all of the pieces here fit together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is making things even more complex, also for the users. As there are not only input_mapping_fields
to be considered but also input_mapping_default_values
.
I think your design proposal would be look like:
resource "azurerm_evengrid_topic" "example" {
...
enable_event_grid_schema = true // Optional, Default, ConflictsWith ["enable_cloud_event_schema_v10", "custom_event_schema_input_mapping"]
enable_cloud_event_schema_v10 = true // Optional, ConflictsWith ["enable_event_grid_schema", "custom_event_schema_input_mapping"]
custom_event_schema_input_mapping { // Optional, ConflictsWith ["enable_event_grid_schema", "enable_cloud_event_schema_v10"]
mapping_fields {
id = "path.id" // optional
topic= "path.topic" // optional
event_time= "path.eventtime" // optional
subject = "path.subject" // required either here
event_type = "path.type" // required either here
data_version = "path.version" // required either here
}
default_mappings {
subject = "foo" // or required here
event_type = "bar" // or required here
data_version = "" // or required here
}
}
}
So far I've aligned the design with the existing resource azurerm_eventgrid_domain
. (see https://www.terraform.io/docs/providers/azurerm/r/eventgrid_domain.html#input_schema)
LMKWYT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your design is what I was thinking but you're right that the burden is a little too high on both sides. I appreciate that you took the time to write it out though.
I wasn't aware that azurerm_eventgrid_domain
also did something similar so I'm a fan of keeping things consistent between that resource and this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
azurerm_evengrid_topic
azurerm_eventgrid_topic
- support for input_schema
, input_mapping_fields
, and input_mapping_default_values
…g_fields`, and `input_mapping_default_values` (hashicorp#6858)
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! |
Adds support for customized input schema and mapping to resource
azurerm_eventgrid_topic
.Additions to resource
azurerm_eventgrid_topic
: