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.

Sign up for GitHub

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

Jump to bottom

Added client certificate exclusion paths app service setting #16603

Merged
merged 2 commits into from
Oct 18, 2022
Merged

Added client certificate exclusion paths app service setting #16603

merged 2 commits into from
Oct 18, 2022

Conversation

olofattemo
Copy link
Contributor

Allows setting the "client_cert_exclusion_paths" on app service web and function apps as suggested in:
#8876

katbyte
katbyte requested changes May 5, 2022
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks for the pr @olofattemo, overall this looks great! my only comments being that we should change the property names (left a comment inline) and that the docs need updating. once that is resolved this should be good to merge!

internal/services/appservice/linux_function_app_data_source.go Outdated Show resolved Hide resolved
internal/services/appservice/linux_function_app_resource.go Outdated
@@ -190,6 +191,12 @@ func (r LinuxFunctionAppResource) Arguments() map[string]*pluginsdk.Schema {
Description: "The mode of the Function App's client certificates requirement for incoming requests. Possible values are `Required`, `Optional`, and `OptionalInteractiveUser` ",
},

"client_cert_exclusion_paths": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

internal/services/appservice/linux_function_app_slot_resource.go Outdated
@@ -173,6 +174,12 @@ func (r LinuxFunctionAppSlotResource) Arguments() map[string]*pluginsdk.Schema {
Description: "The mode of the Function App Slot's client certificates requirement for incoming requests. Possible values are `Required`, `Optional`, and `OptionalInteractiveUser`.",
},

"client_cert_exclusion_paths": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

and for the rest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@olofattemo olofattemo marked this pull request as draft May 13, 2022 18:13
@olofattemo
Copy link
Contributor Author

olofattemo commented May 13, 2022 •
edited
Loading

Ok @katbyte. Thanks for the review. Looks like I confused the naming of the attributes in TF with the API which appears to use a shortened version. Definitively prefer the long names. Will update the docs and change the name of the property name. Going to draft until resolved.

@olofattemo olofattemo force-pushed the 8876-app-service-client-cert-exclusion-paths branch from e2200fc to 4add667 Compare May 19, 2022 18:44
@olofattemo olofattemo marked this pull request as ready for review May 19, 2022 18:46
@olofattemo olofattemo requested a review from katbyte May 19, 2022 18:47
katbyte
katbyte requested changes May 23, 2022
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @olofattemo - we definitely do try and use the name that provides the best UI. looks like we have some test failures to fix:

image

@olofattemo olofattemo force-pushed the 8876-app-service-client-cert-exclusion-paths branch from 4add667 to eed07d5 Compare May 24, 2022 05:08
@olofattemo
Copy link
Contributor Author

Ok @katbyte I fixed the formatting. I assumed the fmtcheck target in the makefile would catch this but I needed to be more thorough

@olofattemo olofattemo requested a review from katbyte May 25, 2022 16:24
@olofattemo olofattemo force-pushed the 8876-app-service-client-cert-exclusion-paths branch from eed07d5 to e0396c0 Compare May 26, 2022 17:12
@github-actions github-actions bot added size/XL and removed size/L labels May 26, 2022
katbyte
katbyte requested changes Jun 9, 2022
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @olofattemo - looks like we have some test failures now thou
image

@jackofallops jackofallops force-pushed the 8876-app-service-client-cert-exclusion-paths branch from e0396c0 to f76d8d7 Compare October 12, 2022 07:59
@jackofallops
Copy link
Member

Hi @olofattemo - Hope you don't mind, but I pushed some changes to your branch to resolve the merge conflicts and fix the test issues. I'll loop back asap to re-review.

katbyte
katbyte approved these changes Oct 18, 2022
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

LGTM 🍄

@jackofallops jackofallops merged commit b7bccee into hashicorp:main Oct 18, 2022
@github-actions github-actions bot added this to the v3.28.0 milestone Oct 18, 2022
jackofallops added a commit that referenced this pull request Oct 18, 2022
@jackofallops
Update for #16603
336e442
@github-actions
Copy link

This functionality has been released in v3.28.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!

@github-actions
Copy link

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 Nov 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@katbyte katbyte katbyte approved these changes

Assignees

@jackofallops jackofallops

Labels
documentation service/app-service size/XL
Projects
None yet
Milestone
v3.28.0
Development

Successfully merging this pull request may close these issues.
3 participants
@olofattemo @jackofallops @katbyte