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

Remove incorrect validation of app service sticky setting names #17209

Merged

Conversation

jennings
Copy link
Contributor

This validation is too strict. The Azure Portal allows the following name for both app settings and connection strings:

A test: !@#$%^&*()_+-=' ";/?

I can't find a reference for what the valid character set would be, and the names in the app_settings attribute aren't validated at all, so we may as well remove this validation.

Fixes #16612

image

This validation is too strict. The Azure Portal allows the following
name for both app settings and connection strings:

    A test: !@#$%^&*()_+-=' ";/?

I can't find a reference for what the valid character set would be,
and the names in the app_settings attribute aren't validated at all,
so we may as well remove this validation.

Fixes hashicorp#16612
@jennings jennings changed the title remove validation of app service sticky setting names Remove incorrect validation of app service sticky setting names Jun 21, 2022
@jennings
Copy link
Contributor Author

@jackofallops I would love to get this into an upcoming release because it's blocking my use of sticky_settings entirely. Is there any research I can to do make this easier to review? Or, let me know if I should change something and I can work on it.

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

HI @jennings - Apologies for the delay in review, I've been away for a few days.

Thanks for the catch here, the information I developed this against suggested this was the correct validation, but I should have remembered your point about setting hierarchy, my bad!

Just two minor changes needed, since we can't remove validation entirely here.

Can you add test(s) for these names that cover special characters? Since this is something that can potentially change over time, we'd need to ensure that's covered. Once those changes and tests are in I'll run them on our acceptance suite and we'll get this merged.

Thanks again!

@github-actions github-actions bot added size/M and removed size/XS labels Jun 22, 2022
@jennings
Copy link
Contributor Author

@jackofallops I added validation.StringIsNotEmpty and added my pathological setting name to the four acceptance tests I found that are testing sticky_settings on azurerm_windows_web_app and azurerm_windows_function_app. I hope this is what you intended, if not I can try again. Thanks for your help!

@jennings jennings requested a review from jackofallops June 23, 2022 00:03
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Hi @jennings - Thanks for making those changes, this LGTM now 👍

@jackofallops jackofallops added this to the v3.12.0 milestone Jun 24, 2022
@jackofallops
Copy link
Member

Tests looking fine:
image

Failure here was a transient issue:
image

@jackofallops jackofallops merged commit 0cebe71 into hashicorp:main Jun 24, 2022
jackofallops added a commit that referenced this pull request Jun 24, 2022
@jennings jennings deleted the remove-sticky-settings-naming-validation branch June 24, 2022 15:17
@github-actions
Copy link

github-actions bot commented Jul 1, 2022

This functionality has been released in v3.12.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

sergelogvinov pushed a commit to sergelogvinov/terraform-provider-azurerm that referenced this pull request Jul 11, 2022
sergelogvinov pushed a commit to sergelogvinov/terraform-provider-azurerm that referenced this pull request Jul 11, 2022
@github-actions
Copy link

github-actions bot commented Aug 1, 2022

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2022
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.

azurerm_windows_web_app sticky_settings app_setting_names doesnt allow valid characters
2 participants