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

Bugfix - Allow configuration of disableChallengeResourceVerification property of AKV SecretClient #36603

Conversation

nagyesta
Copy link
Contributor

@nagyesta nagyesta commented Aug 30, 2023

Description

Fixes #36561

  • Adds challengeResourceVerificationEnabled property to properties objects
  • Includes new property in mapping methods
  • Configures SecretClient in Factory when challengeResourceVerificationEnabled is set
  • Configures CertificateClient in Factory when challengeResourceVerificationEnabled is set
  • Updates/adds new tests
  • Updates Changelog

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

Netyyyy and others added 3 commits August 25, 2023 16:29
* Update versions manually

* Update version/changelog/readme
* Increment package versions for spring releases

* Update pom.xml

* Update version_client.txt

---------

Co-authored-by: Muyao Feng <92105726+Netyyyy@users.noreply.github.com>
- Adds disableChallengeResourceVerification property to properties objects
- Includes new property in mapping methods
- Configures SecretClient in Factory when disableChallengeResourceVerification is set
- Configures CertificateClient in Factory when disableChallengeResourceVerification is set
- Updates/adds new tests
- Updates Changelog

Resolves Azure#36561

Signed-off-by: Esta Nagy <nagyesta@gmail.com>
@github-actions github-actions bot added azure-spring All azure-spring related issues Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Aug 30, 2023
@github-actions
Copy link

Thank you for your contribution @nagyesta! We will review the pull request and get back to you soon.

@nagyesta
Copy link
Contributor Author

@microsoft-github-policy-service agree

@saragluna
Copy link
Member

@nagyesta thanks for the PR, this PR looks great, I left some comments to your PR, please check.

… - Code review fixes #1

- Renames disableChallengeResourceVerification to challengeResourceVerificationEnabled
- Adds additional JavaDoc

Resolves Azure#36561

Signed-off-by: Esta Nagy <nagyesta@gmail.com>
@nagyesta
Copy link
Contributor Author

Hi @saragluna ,
I have completed the requested changes, please let me know in case anything else is needed.
Thank you!

- Fix a missed JavaDoc

Signed-off-by: Esta Nagy <nagyesta@gmail.com>
@saragluna saragluna changed the base branch from release/spring-cloud-azure_5.5.0 to feature/spring-boot-3 September 1, 2023 07:36
@saragluna
Copy link
Member

@nagyesta I changed the base branch to feature/spring-boot-3, which is our base branch for Spring Boot 3 development.

@saragluna
Copy link
Member

@nagyesta I pushed one commit to your PR, the changes including:

@saragluna
Copy link
Member

@nagyesta I left two more comments.

… - Code review fixes Azure#3

- Simplifies factory method logic as per code review recommendation

Resolves Azure#36561

Signed-off-by: Esta Nagy <nagyesta@gmail.com>
@nagyesta
Copy link
Contributor Author

nagyesta commented Sep 1, 2023

Hi @saragluna ,
thank you for making the changes you have mentioned! I have updated the branch with the requested simplifications in the factory calls. Please let me know in case further changes are required!

Regarding the base branch, I wasn't aware of it sorry! Will it be a problem that the PR now contains the version bump specific changes from the release branch?

@saragluna
Copy link
Member

Thanks @nagyesta, no worries, the version bump is already in the base branch, so it won't be an issue.

@saragluna
Copy link
Member

@Netyyyy, let's cherry pick this feature to the main branch.

@saragluna saragluna merged commit a0d30ff into Azure:feature/spring-boot-3 Sep 2, 2023
16 checks passed
@nagyesta nagyesta deleted the bugfix/36561-disable-challenge-resource-verification-akv-_on_5-5-0 branch September 5, 2023 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
azure-spring All azure-spring related issues Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants