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

Issue#858 Add Vnet to Storage Account #904

Merged
merged 36 commits into from
Apr 17, 2020
Merged

Issue#858 Add Vnet to Storage Account #904

merged 36 commits into from
Apr 17, 2020

Conversation

buhongw7583c
Copy link
Contributor

@buhongw7583c buhongw7583c commented Apr 8, 2020 •
edited
Loading

Closes #858

What this PR does / why we need it:
This PR is to add Vnet functions to storage account, and also handle the "asynchronous operation has not completed" in a graceful way.

Special notes for your reviewer:

  • Add the network rules when deploy storage account. With the successful deployment, you should be able to get the virtual network information like this.

image

  • Since current service operator cannot automatically create private endpoint, in the controller test, not able to test the 'happy' path to create storage account with network rule, so could only test the failure case instead. Will open another task to complete the controller test.
  • Using polling url, the 'asynchronous operation has not completed' error shall be handled in a good manner and shouldn't produce confusing message in the status.

How does this PR make you feel:
gif

If applicable:

  • this PR contains documentation
  • this PR contains tests

@jananivMS
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

buhongw7583c and others added 13 commits April 9, 2020 11:09
@buhongw7583c
Merge branch 'master' into issue#858
d681d4c
@buhongw7583c
:Merge branch 'master' of https://github.com/Azure/azure-service-oper…
f7e8d6a 
…ator into issue#858
@buhongw7583c
#issue858#Controllertest
400a0dd
@buhongw7583c
:1Merge branch 'issue#858' of https://github.com/buhongw7583c/azure-s…
3bb8cb0 
…ervice-operator into issue#858
@jananivMS
updates
182a66a
@buhongw7583c
Merge pull request #1 from jananivMS/hongstortest
d5a30c2 
updates from debugging
@buhongw7583c
#issue858#StorageControllertest#HandleAsyncError
4a89de5
@buhongw7583c
Merge branch 'master' into issue#858
b7bbc9f
@buhongw7583c
Merge branch 'master' of https://github.com/Azure/azure-service-operator
ea92543 
 into issue#858
@buhongw7583c
#issue858#conflict resolve
9c0587b
@buhongw7583c
Merge branch 'issue#858' of https://github.com/buhongw7583c/azure-ser…
5721b5b 
…vice-operator into issue#858
@buhongw7583c
#issue858#updatecontroller test
6861470
@buhongw7583c
#issue858#remove unused code
c0fdecb
@frodopwns frodopwns self-requested a review April 10, 2020 16:12
frodopwns
frodopwns reviewed Apr 13, 2020
View reviewed changes
Copy link
Contributor

@frodopwns frodopwns left a comment

Choose a reason for hiding this comment

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

some initial comments before I get into actual testing

pkg/resourcemanager/storages/storageaccount/storage_reconcile.go Outdated Show resolved Hide resolved
config/samples/azure_v1alpha1_storage.yaml Outdated Show resolved Hide resolved
config/samples/azure_v1alpha1_storage.yaml Outdated Show resolved Hide resolved
pkg/resourcemanager/storages/storageaccount/storage.go Outdated Show resolved Hide resolved
frodopwns
frodopwns reviewed Apr 15, 2020
View reviewed changes
config/default/manager_image_patch.yaml-e Outdated
optional: true
#requeue after time in seconds"
- name: REQUEUE_AFTER
value: "30"
Copy link
Contributor

Choose a reason for hiding this comment

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

this file should be removed...I think the -e files are related to errors

frodopwns
frodopwns approved these changes Apr 16, 2020
View reviewed changes
Copy link
Contributor

@frodopwns frodopwns left a comment

Choose a reason for hiding this comment

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

lgtm

@buhongw7583c buhongw7583c merged commit 5b957e6 into Azure:master Apr 17, 2020
@buhongw7583c buhongw7583c deleted the issue#858 branch April 17, 2020 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frodopwns frodopwns frodopwns approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Task: Add ability for storage account to be configured in specified VNET
4 participants
@buhongw7583c @jananivMS @frodopwns @WilliamMortlMicrosoft