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

Tier Options to KeyVault #853

Merged
merged 12 commits into from
Apr 1, 2020
Merged

Tier Options to KeyVault #853

merged 12 commits into from
Apr 1, 2020

Conversation

melonrush13
Copy link
Contributor

@melonrush13 melonrush13 commented Mar 31, 2020

Closes #600

What this PR does / why we need it:
When creating a KeyVault on the Azure portal, users can create a KV of tier options "Standard" and "Premium"
This PR allows user to have the flexibility to choose one of these options in the yaml.

Special notes for your reviewer:
Create a KV with sku.Name = "Premium" in yaml
Create a KV with sku.Name = "Standard" in yaml

Verify it exists in portal with proper tier
Screen Shot 2020-03-31 at 1 04 47 PM

How does this PR make you feel:
gif

If applicable:

  • this PR contains documentation
  • this PR contains tests

@melonrush13
Copy link
Contributor Author

melonrush13 commented Mar 31, 2020

My question to reviewer / whoever: When we create a keyvault with access policies, this is done through the function CreateVault, and access policies are added inside of that function.

That being said, we have a function that is called CreateVaultWithAccessPolicies() which is only used during tests.

I currently added the value sku to this function, (CreateVaultWithAccesPolicies()) but am unsure if it is needed since Keyvault_simple.yaml and keyvault.yaml both use CreateVault() to create a KV.

Thoughts? I think I want to remove it personally :)

@jananivMS
Copy link
Contributor

jananivMS commented Mar 31, 2020

My question to reviewer / whoever: When we create a keyvault with access policies, this is done through the function CreateVault, and access policies are added inside of that function.

That being said, we have a function that is called CreateVaultWithAccessPolicies() which is only used during tests.

I currently added the value sku to this function, (CreateVaultWithAccesPolicies()) but am unsure if it is needed since Keyvault_simple.yaml and keyvault.yaml both use CreateVault() to create a KV.

Thoughts? I think I want to remove it personally :)

Yes, this is only used in tests. I think we initially had it but then we ended up using the CreateVault one. @cnadolny can you confirm? If we remove we should make sure we dont break those tests though.

@jpflueger jpflueger self-requested a review March 31, 2020 19:50
@cnadolny
Copy link
Contributor

My question to reviewer / whoever: When we create a keyvault with access policies, this is done through the function CreateVault, and access policies are added inside of that function.
That being said, we have a function that is called CreateVaultWithAccessPolicies() which is only used during tests.
I currently added the value sku to this function, (CreateVaultWithAccesPolicies()) but am unsure if it is needed since Keyvault_simple.yaml and keyvault.yaml both use CreateVault() to create a KV.
Thoughts? I think I want to remove it personally :)

Yes, this is only used in tests. I think we initially had it but then we ended up using the CreateVault one. @cnadolny can you confirm? If we remove we should make sure we dont break those tests though.

Yep - just used in tests. I think it was originally added before we added access policy support to the keyvault spec, removing it and replacing it with CreateVault should be fine.

jananivMS and others added 2 commits March 31, 2020 20:35
fixes nil pointer dereference (includes suggestions to make the sku type simpler)
@frodopwns frodopwns requested a review from jpflueger April 1, 2020 16:16
Copy link
Contributor

@jpflueger jpflueger left a comment

Choose a reason for hiding this comment

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

code looks good to me, @jananivMS answered my questions via teams

@jananivMS jananivMS merged commit c5fb4e8 into Azure:master Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task: Allow Tier to be set for KeyVault in the Spec
4 participants