-
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
New Data Source: azurerm_eventhub_sas #22215
Conversation
@katbyte , thanks for the comment. I replied it. Please take another look. Thanks. |
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.
Per my inline comment above, I think the escape sequence issue should be fixed in the data source (or helper func, or SDK).
b021179
to
37de341
Compare
@manicminer , thanks for the comment. I replied it. Please take another look. Thanks. |
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.
Thanks @neil-yechenwei, is there a reason this is an untyped data source? New data sources should use the typed SDK unless there is a compelling reason not to.
@manicminer , I made this PR on my local about 2 years ago and this PR depends on another PR which is also submitted about 2 years ago. Given another PR is merged recently, so I submitted this PR. So shall we merge this PR first? Once I have bandwidth, I will submit another PR to convert it to typed resource. Does it make sense? |
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.
@neil-yechenwei Unfortunately I don't believe we can merge this as a new untyped resource. Can you please refactor this as a typed resource? Apologies I didn't notice this earlier in the review cycle.
@manicminer , I've converted it to typed resource. Please take another look. Thanks. |
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.
Thanks for updating @neil-yechenwei, this LGTM 👍
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. |
fixes #7160