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

Google Cloud Storage Import Support #2615

Merged
merged 4 commits into from
Mar 22, 2023
Merged

Google Cloud Storage Import Support #2615

merged 4 commits into from
Mar 22, 2023

Conversation

feitnomore
Copy link
Contributor

What this PR does / why we need it: This PR introduces support for Google Cloud Storage as Source for DataVolume.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

This introduces support for Google Cloud Storage as a source for DataVolumes.
The GCS Source supports authentication through a JSON key file, used as secret.
URLs are supported in two formats:
    gs://bucket/file.ext
    https://storage.cloud.google.com/bucket/file.ext
There is unit-test coverage as well as two functional tests.

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 1, 2023
@kubevirt-bot kubevirt-bot requested review from maya-r and mhenriks March 1, 2023 22:12
@kubevirt-bot
Copy link
Contributor

Hi @feitnomore. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@feitnomore
Copy link
Contributor Author

@awels , these are the changes I've been working on. Would appreciate your inputs.

@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2023
@awels
Copy link
Member

awels commented Mar 2, 2023

/retest-required

@akalenyu
Copy link
Collaborator

akalenyu commented Mar 2, 2023

/ok-to-test

@kubevirt-bot kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 2, 2023
@maya-r
Copy link
Contributor

maya-r commented Mar 6, 2023

/retest

@feitnomore
Copy link
Contributor Author

/retest

awels
awels reviewed Mar 7, 2023
View reviewed changes
Copy link
Member

@awels awels left a comment

Choose a reason for hiding this comment

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

Hey thanks for the PR. I have a few comments and one question. I see you added a new secret field for the credentials. Is there a reason the existing secretRef is not sufficient? There is no pre-set format in that secret, so it should work for the json credentials.

pkg/apiserver/webhooks/datavolume-validate.go Outdated Show resolved Hide resolved
pkg/importer/gcs-datasource.go Outdated Show resolved Hide resolved
pkg/importer/gcs-datasource.go Outdated Show resolved Hide resolved
pkg/importer/gcs-datasource.go Show resolved Hide resolved
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2023
Google Cloud Storage Importer
65c695c 
This is a Google Cloud Storage importer for CDI

Signed-off-by: Marcelo Parisi <marcelo@feitoza.com.br>
@feitnomore feitnomore force-pushed the main branch 2 times, most recently from af5a342 to 65c695c Compare March 19, 2023 16:11
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 19, 2023
Fix auto-generated swagger and openapi
a679c1f 
Signed-off-by: Marcelo Parisi <marcelo@feitoza.com.br>
@awels
Copy link
Member

awels commented Mar 20, 2023

/test pull-cdi-generate-verify

@awels
Copy link
Member

awels commented Mar 20, 2023

/test pull-containerized-data-importer-fossa

awels
awels reviewed Mar 20, 2023
View reviewed changes
Copy link
Member

@awels awels left a comment

Choose a reason for hiding this comment

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

Looks good a few minor issues I noted.

pkg/apiserver/webhooks/datavolume-validate_test.go Outdated Show resolved Hide resolved
pkg/apiserver/webhooks/datavolume-validate_test.go Outdated Show resolved Hide resolved
pkg/importer/gcs-datasource.go Show resolved Hide resolved
tests/images/gcs-secret.txt Show resolved Hide resolved
Marcelo Parisi added 2 commits March 20, 2023 18:16
GCS Importer General Fixes
5e65d97 
Signed-off-by: Marcelo Parisi <marcelo@feitoza.com.br>
Moving back gcs-secret.txt
71138d9 
Moving file back to imageDir to fix unit testing.

Signed-off-by: Marcelo Parisi <marcelo@feitoza.com.br>
awels
awels reviewed Mar 21, 2023
View reviewed changes
Copy link
Member

@awels awels left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2023
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 21, 2023
@awels
Copy link
Member

awels commented Mar 22, 2023

/retest-required

@kubevirt-bot kubevirt-bot merged commit c7467cc into kubevirt:main Mar 22, 2023
@EduardGomezEscandell EduardGomezEscandell mentioned this pull request Apr 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akalenyu akalenyu akalenyu left review comments

@awels awels awels left review comments

@maya-r maya-r Awaiting requested review from maya-r

@mhenriks mhenriks Awaiting requested review from mhenriks

Assignees

@awels awels

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@feitnomore @kubevirt-bot @awels @akalenyu @maya-r @mhenriks