-
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_synapse_workspace
: fix panic of slice index out-of-range
#23019
Conversation
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.
Hi @wuxu92, thank you for opening this PR. While the parsing code would fix the panic mentioned in the issue I think the real fix would be to add a ValidateFunc
to the storage_data_lake_gen2_filesystem_id
field in the schema. That way if the end users pass in an invalid storage_data_lake_gen2_filesystem_id
it would be caught during plan
phase of deployment. What do you think?
@WodansSon thanks for your review. a validation func added to this field. |
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 :
Type: pluginsdk.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
ValidateFunc: validation.IsURLWithPath, |
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.
this is a resource id? should we not be validating it as an id not a uri?
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.
@katbyte, the ID here is in the form of a URI (e.g., https://storageaccountname.dfs.core.windows.net/filesystemname
)
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.
Hi @wuxu92, thanks for pushing those changes, this LGTM now! 🚀
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. |
Not sure why need a
uri.Path[1:]
here, I guess it want to trim the leading slash (by the comment inline), so change to usestrings.TrimPrefix
fixes: #23004