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

Revendor azure SDk #76

Merged
merged 3 commits into from Dec 5, 2018
Merged

Revendor azure SDk #76

merged 3 commits into from Dec 5, 2018

Conversation

swapnilgm
Copy link
Contributor

@swapnilgm swapnilgm commented Nov 29, 2018 •
edited

What this PR does / why we need it:
As per doc it is recommended to use package github.com/Azure/azure-storage-blob-go as azure storage SDK, this PR re-vendors it.

Which issue(s) this PR fixes:
Fixes #74
Partially completes #70

Special notes for your reviewer:

Release note:

Updated the azure sdk as per the recommendation in [doc](https://github.com/Azure/azure-sdk-for-go/tree/master/storage#azure-storage-sdk-for-go-preview) to use  package `github.com/Azure/azure-storage-blob-go` as azure storage SDK,

@swapnilgm swapnilgm added kind/enhancement Enhancement, improvement, extension exp/beginner Issue that requires only basic skills reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies platform/az needs/review Needs review size/s Size of pull request is small (see gardener-robot robot/bots/size.py) component/etcd-backup-restore ETCD Backup & Restore needs/lgtm Needs approval for merging area/backup Backup related needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) status/new Issue is new and unprocessed labels Nov 29, 2018
@swapnilgm swapnilgm force-pushed the azure-revendor branch 2 times, most recently from 7a2dc98 to 83ef838 Compare November 30, 2018 12:10
@swapnilgm swapnilgm added status/in-progress Issue is in progress/work and removed reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies status/new Issue is new and unprocessed labels Nov 30, 2018
Swapnil Mhamane added 2 commits December 2, 2018 23:25
Revendor azure sdk to azure azure-storage-blob-go
3c671ee 
Signed-off-by: Swapnil Mhamane <swapnil.mhamane@sap.com>
Add mock test for azure
78a6753 
Signed-off-by: Swapnil Mhamane <swapnil.mhamane@sap.com>
@swapnilgm swapnilgm added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 4, 2018
shreyas-s-rao
shreyas-s-rao requested changes Dec 4, 2018
View reviewed changes
Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao 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 well-written PR! I have suggested a few changes. Can you please address them?

LICENSE.md Outdated Show resolved Hide resolved
LICENSE.md Outdated Show resolved Hide resolved
pkg/snapstore/abs_snapstore.go Outdated Show resolved Hide resolved
pkg/snapstore/abs_snapstore_test.go Outdated Show resolved Hide resolved
pkg/snapstore/abs_snapstore_test.go Show resolved Hide resolved
pkg/snapstore/abs_snapstore.go
client := storageClient.GetBlobService()

// GetABSSnapstoreFromClient returns a new ABS object for a given container using the supplied storageClient
func GetABSSnapstoreFromClient(container, prefix, tempDir string, maxParallelChunkUploads int, containerURL *azblob.ContainerURL) (*ABSSnapStore, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be better to rename this function? Because it now uses containerURL instead of a client to return the snapstore object.

pkg/snapstore/abs_snapstore_test.go Show resolved Hide resolved
@shreyas-s-rao
Copy link
Collaborator

Also, regarding https://github.com/gardener/etcd-backup-restore/blame/master/LICENSE.md#L190.

Copyright [yyyy] [name of copyright owner]

Shouldn't we have filled this?

@swapnilgm
Copy link
Contributor Author

Also, regarding https://github.com/gardener/etcd-backup-restore/blame/master/LICENSE.md#L190.
Copyright [yyyy] [name of copyright owner]
Shouldn't we have filled this?

I guess this is how license file is defined eveywhere. May be @msohn can give better reply to this.

@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 4, 2018
@swapnilgm
Copy link
Contributor Author

@shreyas-s-rao Thank you for the review. I have adopted the suggestions in PR. PTAL.

@swapnilgm swapnilgm added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 4, 2018
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 4, 2018
@swapnilgm swapnilgm added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review labels Dec 5, 2018
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 5, 2018
georgekuruvillak
georgekuruvillak suggested changes Dec 5, 2018
View reviewed changes
Copy link
Contributor

@georgekuruvillak georgekuruvillak left a comment

Choose a reason for hiding this comment

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

lgtm

pkg/snapstore/s3_snapstore_test.go Outdated Show resolved Hide resolved
pkg/snapstore/abs_snapstore_test.go Outdated Show resolved Hide resolved
@georgekuruvillak georgekuruvillak added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging and removed needs/lgtm Needs approval for merging needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 5, 2018
Fix s3 snapstore data race condition
a3e612f 
Signed-off-by: Swapnil Mhamane <swapnil.mhamane@sap.com>
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 5, 2018
@swapnilgm
Copy link
Contributor Author

Thanks you for Review @georgekuruvillak. I have updated the PR. PTAL.

@georgekuruvillak georgekuruvillak merged commit 4706e5e into gardener:master Dec 5, 2018
@swapnilgm swapnilgm deleted the azure-revendor branch December 5, 2018 05:03
swapnilgm pushed a commit to swapnilgm/etcd-backup-restore that referenced this pull request Dec 7, 2018
@georgekuruvillak
Merge pull request gardener#76 from swapnilgm/azure-revendor
8d36495 
Revendor azure SDk

Signed-off-by: Swapnil Mhamane <swapnil.mhamane@sap.com>
@PadmaB PadmaB modified the milestones: ---, 0.4.0 Jan 23, 2019
@ghost ghost added the platform/azure Microsoft Azure platform/infrastructure label Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shreyas-s-rao shreyas-s-rao shreyas-s-rao requested changes

@georgekuruvillak georgekuruvillak georgekuruvillak approved these changes

Assignees
No one assigned
Labels
area/backup Backup related component/etcd-backup-restore ETCD Backup & Restore exp/beginner Issue that requires only basic skills kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) platform/azure Microsoft Azure platform/infrastructure reviewed/lgtm Has approval for merging size/s Size of pull request is small (see gardener-robot robot/bots/size.py) status/in-progress Issue is in progress/work
Projects
None yet
Milestone
0.4.0
Development

Successfully merging this pull request may close these issues.

Update Azure sdk path to github.com/Azure/azure-storage-blob-go
5 participants
@swapnilgm @shreyas-s-rao @georgekuruvillak @PadmaB @gardener-robot-ci-1