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

Add initial support for gsutil and pulling from Google Storage buckets #421

Merged
merged 4 commits into from
Aug 31, 2020
Merged

Add initial support for gsutil and pulling from Google Storage buckets #421

merged 4 commits into from
Aug 31, 2020

Conversation

j0sh3rs
Copy link
Contributor

@j0sh3rs j0sh3rs commented Aug 24, 2020

Pull Request (PR) description

This PR adds support in the archive module for pulling assets from Google Storage buckets by way of gsutil

This Pull Request (PR) fixes the following issues

No Issues were filed prior to this PR.

@j0sh3rs j0sh3rs marked this pull request as ready for review August 24, 2020 15:18
@bastelfreak bastelfreak added the enhancement New feature or request label Aug 26, 2020
@bastelfreak
Copy link
Member

@j0sh3rs thanks for the PR. Can you add some unit tests for this change?

@j0sh3rs
Copy link
Contributor Author

j0sh3rs commented Aug 28, 2020

@bastelfreak -- Added some tests, let me know if they're up to par! I basically copied the ruby spec and modified it for gsutil.

Copy link
Member

@bastelfreak bastelfreak 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 to me, but I'm not a types/provider expert. @alexjfisher or @ekohl would you like to review as well?

Copy link
Contributor

@igalic igalic left a comment

Choose a reason for hiding this comment

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

the provider and the tests look good
I'm just not sure about gsutil itself

@@ -66,4 +69,32 @@
}
}
}

if $gsutil_install {
# TODO: Windows support.
Copy link
Contributor

Choose a reason for hiding this comment

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

https://cloud.google.com/storage/docs/gsutil_install?hl=en#windows
installation on windows uses the same archive, but different paths
and python installation on windows isn't guaranteed, so that's another issue, and so might the execution be

however, given that this thing requires python 2.7, which is EOL, a safe execution on any platform is not guaranteed

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

would be cool if they updated their documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @igalic -- totally agreed that the doc on Google's side could use some work (this is seemingly systemic for them). Are we considering this a blocker for acceptance of this PR, or is there something additional I can provide to help shepherd this through?

Thanks very much for the review.

Copy link
Contributor

Choose a reason for hiding this comment

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

it was a remark on my approving of this pr

so i think with enough eyes on this now, we can hit the merge button and see what happens after that

@igalic igalic merged commit 49f66f9 into voxpupuli:master Aug 31, 2020
cegeka-jenkins pushed a commit to cegeka/puppet-archive that referenced this pull request Mar 26, 2021
Add initial support for gsutil and pulling from Google Storage buckets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants