-
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
Merged
mbfrahry
merged 4 commits into
hashicorp:master
from
jrauschenbusch:r-eventgrid-topic-input-mapping
May 26, 2020
Merged
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
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 theOptional
flag. However, it must be optional because the input_mapping is only used when the input_schema is changed toCustomEventSchema
.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
orCloudEventV01Schema
as input_schema, and we add an (empty) input_mapping_fields and/or input_mapping_default_values block, the following error will be reported:When using
CustomEventSchema
as input_schema and adding empty input_mapping_fields and input_mapping_default_values blocks the following will be reported:After adding a sourceField or defaultValue for
data_version
the following is reported:After adding a sourceField or defaultValue for
subject
the following is reported: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:
input_schema != "CustomEventSchema"
, then the blocks input_mapping_fields and input_mapping_default_values must not be presentinput_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
andInputSchemaEventGridSchema
can just be booleans that conflict with each other likeis_event_grid_schema
and then havecustom_event_schema_input_mapping
with all of these variables in there. That will let us better differentiate theRequired
andOptional
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 alsoinput_mapping_default_values
.I think your design proposal would be look like:
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.