Skip to content
This repository has been archived by the owner on Oct 30, 2018. It is now read-only.

win_s3 Module #180

Closed
wants to merge 6 commits into from
Closed

win_s3 Module #180

wants to merge 6 commits into from

Conversation

schwartzmx
Copy link
Contributor

win_s3 module:

  • Requires AWS_SDK for PowerShell
    • If the AWSPowerShell module is not found on the machine, the AWS_SDK tools will be downloaded and installed before continuing. Read more about AWS_SDK: AWS-PowerShell

Credentials:

  • The module allows for access_key and secret_key params to be used as the session's AWS Credentials. If these are not supplied, the AWS_SDK will search the machine for configured credentials, either through IAM role, environment variables, or aws config file. Read more: Credentials-Search-Order

Functions:

  • Upload a file, or entire directory to s3 with a specified bucket, key, and local file or directory.
  • Download a file or entire directory from s3 with a specified bucket, key, and local file path or directory path. (Will be created on download)
  • * Uploading or Downloading an entire directory requires that the key entered be a key-prefix, in other words not a full path to a file. Such as Key/Directory/To/Be/Downloaded/ as opposed to Key/To/Be/Downloaded.zip.
  • Also specify rm param to remove the local file or directory after upload.

Params:

  • bucket
    • AWS bucket
  • key
    • key or object (path from bucket)
  • local
    • local file or directory
  • method

Supported method params:

  • upload
    • file or directory
  • download
    • file or directory

Other params:

  • access_key
    • AWS ACCESS ID KEY
  • secret_key
    • AWS SECRET ACCESS KEY
  • rm
    • Specify whether to remove local file or directory

I tried to make the exception handling, and resultant error messages as informative and helpful as possible. If the error messages don't help solve the problem when testing, then it is most likely an authorization error for accessing a bucket/key with the machine credentials or specified credentials.

I believe there is room for more functionality in the future, but this does what I need it to do for the time being.

Questions, concerns, suggestions? Please let me know.

Phil

- Decided to do a little more logic, to eliminate the extra method name
- Does more path and variable checking to remove false positives of upload/download
- Fixes an error that can occur with bad format JSON which has to do with an ending '\' which can potentially occur when users specify a local path.
@juliedavila
Copy link
Contributor

To avoid confusion with the pre-existing S3 module, what do you think about writing smth in the documentation along the lines of:
this is intended to be used when you want your target windows machine to be able to DL/UL to S3

I would suggest to change local to path to keep with the conventions of preexisting modules.

Also, you should remove the automatic installation of the AWS SDK. This should instead raise an exception to the user and fail (similar to how the existing AWS modules work and error out when boto isn't detected).

@schwartzmx
Copy link
Contributor Author

These are all good suggestions, I will work on updating the module when I get a chance in the next couple of days.

Update: Also this module doesn't use boto, that's why it installs AWSPowerShell on the host machine to use locally with specified or IAM credentials.

…were provided, and also added overwrite param to explicitly specify whether to overwrite a file of the same name on the host machine. Thanks to @ryanwalls
@gregdek
Copy link
Contributor

gregdek commented Jul 4, 2015

Hi @schwartzmx I saw that you made some revisions here -- is this ready for re-review?

@schwartzmx
Copy link
Contributor Author

@gregdek sorry for the late reply, I had to make a couple minor changes, but this should be ready for re-review now. Thanks.

@gregdek
Copy link
Contributor

gregdek commented Sep 25, 2015

Adding new process boilerplate. We will be evaluating all new module PRs according to this process, effective immediately.

Thanks for submitting this new module to Ansible Extras! This module is now in community review, a process that is open to all Ansible users. In order for this module to be approved, it must gain the following votes:

“works_for_me”: If you have tested the module thoroughly, including testing of all of the module’s options, and if the module works for you, please add “works_for_me” in the comments.

“passes_guidelines”: If you have gone through the module guidelines and the module meets all of the requirements, please add “passes_guidelines” in the comments. Guidelines are available here: http://docs.ansible.com/developing_modules.html#module-checklist

“needs_revision”: If the module fails to work for you, or if it doesn’t meet guidelines, please add “needs_revision” in the comments with details about what needs to be fixed.

When a module has both “works_for_me” and “passes_guidelines” tags, we will promote the module for inclusion in Ansible Extras. At this point, you will be expected to maintain the module by fixing bugs and evaluating pull requests in a timely manner.

Thanks again for submitting your Ansible module!

@gregdek
Copy link
Contributor

gregdek commented Sep 25, 2015

Windows friends, please consider a review here: @trondhindenes @petemounce @elventear @smadam813 @jhawkesworth @angstwad @sivel @chrishoffman @cchurch

@gregdek gregdek removed the P3 label Sep 25, 2015
@jhawkesworth
Copy link
Contributor

Can't really comment on the functionality as I'm not an S3 user.

Currently it doesn't meet the guidelines, but the PR actually pre-dates the guidelines so would have needed a crystal ball or similar to do so.

To meet guidelines, I'd suggest...
Switching the parameter checking to use Get-AnsibleParam and adding author github id.ck
check it all still works now that strict-mode is on for ansible modules.

So its a needs_revision at the moment, but looks like it ought not to be lots of work to fix up.

@gregdek
Copy link
Contributor

gregdek commented Oct 28, 2015

Thanks @jhawkesworth.

@schwartzmx Please make the suggested revisions, and when you're done, please add a comment with the text "ready_for_review" and we will continue with the review process. Thanks!

@robynbergeron
Copy link
Contributor

@schwartzmx A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes.

@gregdek
Copy link
Contributor

gregdek commented Jan 26, 2016

@schwartzmx Another friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open. If we don't hear from you within another 14 days, we will close this pull request.

@gregdek
Copy link
Contributor

gregdek commented Mar 7, 2016

Closing due to contributor inactivity.

@gregdek gregdek closed this Mar 7, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants