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

Adding keyvault service (only standard SKU) with key & secret (soft delete feature not supported) #151

Merged
merged 2 commits into from
Mar 17, 2022
Merged

Adding keyvault service (only standard SKU) with key & secret (soft delete feature not supported) #151

merged 2 commits into from
Mar 17, 2022

Conversation

rkhaled0
Copy link
Contributor

This PR aims to provide support for KeyVault service only SKU Standard tier.

In some circumstances meaning custom ASH environment, soft deletion should be configurable.

Additionally, sometimes object_id is not a valid UUID.

@hashicorp-cla
Copy link

hashicorp-cla commented Feb 18, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

katbyte
katbyte requested changes Feb 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.

quick comment while passing through and checking tests:

image

internal/services/keyvault/key_vault_access_policy_resource.go Outdated Show resolved Hide resolved
@rkhaled0
Copy link
Contributor Author

Regarding acceptance tests, I think that we have an unexpected behavior when soft delete feature is enabled.

Do we assume that, at this moment, soft deletion is not supported ?
I don't find any official documentation regarding this situation.

any opinion @katbyte @tombuildsstuff ?

Thank you.

@rkhaled0 rkhaled0 changed the title Adding keyvault service (only standard SKU) with key & secret Adding keyvault service (only standard SKU) with key & secret ( soft delete feature not supported) Feb 24, 2022
@rkhaled0 rkhaled0 changed the title Adding keyvault service (only standard SKU) with key & secret ( soft delete feature not supported) Adding keyvault service (only standard SKU) with key & secret (soft delete feature not supported) Feb 24, 2022
@rkhaled0 rkhaled0 force-pushed the feat/keyvault-service-standard-with-key-and-secrets branch from 01c1517 to 565e24b Compare February 25, 2022 07:49
@katbyte
Copy link
Collaborator

katbyte commented Mar 1, 2022

@redak-pragma - is this something that is not supported in azure stack at all? or a feature that needs to be enabled on a stack instance?

@rkhaled0
Copy link
Contributor Author

rkhaled0 commented Mar 1, 2022

@redak-pragma - is this something that is not supported in azure stack at all? or a feature that needs to be enabled on a stack instance?

@katbyte , not supported at all

katbyte
katbyte requested changes Mar 1, 2022
View reviewed changes
internal/features/user_flags.go Outdated Show resolved Hide resolved
internal/provider/features.go Outdated Show resolved Hide resolved
website/docs/r/key_vault_secret.html.markdown Outdated Show resolved Hide resolved
website/docs/r/key_vault.html.markdown Outdated Show resolved Hide resolved
internal/services/keyvault/key_vault_access_policy_data_source.go Outdated Show resolved Hide resolved
internal/services/keyvault/key_vault_access_policy_data_source_test.go Outdated Show resolved Hide resolved
internal/services/keyvault/key_vault_access_policy_resource.go Outdated Show resolved Hide resolved
internal/services/keyvault/key_vault_access_policy_resource.go Outdated Show resolved Hide resolved
internal/services/keyvault/key_vault_data_source.go Outdated Show resolved Hide resolved
@katbyte
Copy link
Collaborator

katbyte commented Mar 1, 2022

Thanks @redak-pragma - looking better, still have some comments i left inline as well 4 failing tests to address:
image

@rkhaled0
Copy link
Contributor Author

rkhaled0 commented Mar 2, 2022

Thanks @redak-pragma - looking better, still have some comments i left inline as well 4 failing tests to address: image

@katbyte , thank you a ton for reviewing again.
Those acceptance tests were failed because of permission casing: fa05dcc

@rkhaled0 rkhaled0 requested a review from katbyte March 2, 2022 15:29
katbyte
katbyte requested changes Mar 11, 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 @redak-pragma - tests pass now so overall this is good to go aside from needed to remove the commented out code. we don't just leave it in. once thats done i'll get this merged!

@rkhaled0
Adding keyvault service (only standard SKU) with key & secret
d35ecb2 
considering ASH limitations, soft delete is not yet supported
@rkhaled0 rkhaled0 force-pushed the feat/keyvault-service-standard-with-key-and-secrets branch from fa05dcc to d35ecb2 Compare March 14, 2022 20:10
@rkhaled0
Copy link
Contributor Author

Thanks @redak-pragma - tests pass now so overall this is good to go aside from needed to remove the commented out code. we don't just leave it in. once thats done i'll get this merged!

@katbyte , thank you for your review,

Here my recent changes 60f72bb (will be squashed as soon as you review again this PR, thank you in advance.)

katbyte
katbyte approved these changes Mar 17, 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 @redak-pragma - no need as i'll squash when i merge. LGTM now! 🦀

@katbyte katbyte merged commit 2677ab9 into hashicorp:main Mar 17, 2022
@github-actions github-actions bot added this to the v1.0.0 milestone Mar 17, 2022
@TheBlackMini TheBlackMini mentioned this pull request Apr 22, 2022
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 21, 2023
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
No one assigned
Labels
dependencies documentation size/XL
Projects
None yet
Milestone
v1.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@rkhaled0 @hashicorp-cla @katbyte