-
Notifications
You must be signed in to change notification settings - Fork 5
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 a sync file from S3 feature #55
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea a lot. I have a few questions:
- Can you also create documentation for this new command? I'm not sure how to specify the source S3 path. Do we need to include the
s3://
prefix? - Is rclone installed by default on most unix distributions? If not, should we give instructions to the user on how to install it?
- sync and rsync are very similar commands. Is it better to update rsync to first detect if the source path is local or S3 and then use rsync or rclone appropriately?
@@ -142,6 +142,7 @@ def add_action_args(parser): | |||
""" | |||
action_grp = parser.add_argument_group('Action Arguments') | |||
action_grp.add_argument('--rsync_path', '--rsync-path') | |||
action_grp.add_argument('--sync_path', '--rclone-path') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the second string be --sync-path
?
@@ -0,0 +1,107 @@ | |||
"""Rsync user content to EC2 instance.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module docstring needs updating
pem_secret = config['forge_pem_secret'] | ||
region = config['region'] | ||
profile = config.get('aws_profile') | ||
rsync_loc = config.get('sync_path', config.get('app_dir')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change this variable to sync_loc
or rclone_loc
for consistency and to avoid confusion with the rsync command
Add the ability for Forge to sync a file from S3 to an instance. The new
sync
action adds this. It uses rclone instead of rsync to allow for files to be sent from the local machine, s3, or any other cloud storage provider. It is currently set up for AWS only but this can be expanded in future updates.