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 a sync file from S3 feature #55

Closed
wants to merge 6 commits into from
Closed

Add a sync file from S3 feature #55

wants to merge 6 commits into from

Conversation

Macr0Nerd
Copy link
Collaborator

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.

Copy link
Collaborator

@jagmoreira jagmoreira left a 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:

  1. 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?
  2. Is rclone installed by default on most unix distributions? If not, should we give instructions to the user on how to install it?
  3. 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')
Copy link
Collaborator

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."""
Copy link
Collaborator

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'))
Copy link
Collaborator

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

@Macr0Nerd Macr0Nerd closed this Jan 19, 2025
@Macr0Nerd Macr0Nerd deleted the s3_rsync branch January 19, 2025 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants