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_eventgrid_topic - support for input_schema, input_mapping_fields, and input_mapping_default_values #6858

Merged

Conversation

jrauschenbusch
Copy link
Contributor

@jrauschenbusch jrauschenbusch commented May 11, 2020

Adds support for customized input schema and mapping to resource azurerm_eventgrid_topic.

Additions to resource azurerm_eventgrid_topic:

  • input_schema
  • input_mapping_fields
  • input_mapping_default_values

@jrauschenbusch jrauschenbusch changed the title r/evengrid_topic_resource: add input schema support Update Resource: azurerm_evengrid_topic: add input schema support May 12, 2020
@jrauschenbusch jrauschenbusch changed the title Update Resource: azurerm_evengrid_topic: add input schema support Update Resource: add input schema support to azurerm_evengrid_topic May 12, 2020
Copy link
Member

@mbfrahry mbfrahry left a 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

ForceNew: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"id": {
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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", then subject, event_type and data_version are required in one of the input_mapping(_default) blocks.

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM!

@mbfrahry mbfrahry changed the title Update Resource: add input schema support to azurerm_evengrid_topic azurerm_eventgrid_topic - support for input_schema, input_mapping_fields, and input_mapping_default_values May 26, 2020
@mbfrahry mbfrahry merged commit 5da4f81 into hashicorp:master May 26, 2020
mbfrahry added a commit that referenced this pull request May 26, 2020
pbrit pushed a commit to pbrit/terraform-provider-azurerm that referenced this pull request May 31, 2020
pbrit pushed a commit to pbrit/terraform-provider-azurerm that referenced this pull request May 31, 2020
@ghost
Copy link

ghost commented Jun 26, 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 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked and limited conversation to collaborators Jun 26, 2020
@jrauschenbusch jrauschenbusch deleted the r-eventgrid-topic-input-mapping branch July 9, 2020 05:15
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.

None yet

3 participants