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

If the Latest Version of an Object Is the Same Version as What Will Be Restored Skip It. #5

Closed
wants to merge 6 commits into from

Conversation

kylec32
Copy link
Contributor

@kylec32 kylec32 commented Oct 6, 2020

Makes an API call to determine if the latest object is the same one as the one that needs to be restored, if so it will skip the copy. This also introduces a new CLI flag --avoid-duplicates that this new funcationality lives behind, if the flag isn't set the old behavior is preserved.

Sorry for the inudation of PRs. Found this tool and it did 97% of what I needed, wanted to help bring it to 100%. I'm sure I'll have to fix some merge conflicts as they are merged in, happy to do that as we go but wanted to make a PR for each individual solution so we can discuss each in isolation.

Copy link
Owner

@angeloc angeloc left a comment

Choose a reason for hiding this comment

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

I think the feature is good, but you defintely need to squash all the commits into one as the feature is pretty confined.

Could you add some testing to this?

README.md Outdated
@@ -111,6 +112,7 @@ optional arguments:
--test s3 pit restore testing
--max-workers MAX_WORKERS
max number of concurrent download requests
--avoid-duplicates tries to avoid sopying files that are already at the latest version
Copy link
Owner

Choose a reason for hiding this comment

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

fix "sopying" typo

s3-pit-restore Outdated
@@ -253,6 +270,7 @@ def get_key(obj):
return obj["Key"]
return os.path.join(args.dest_prefix, obj["Key"])


Copy link
Owner

Choose a reason for hiding this comment

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

please remove this

@angeloc
Copy link
Owner

angeloc commented Oct 9, 2020

@kylec32 you should squash all commits into one, not adding others on top. The feature is self contained, touches 2 files, so it should be in a single commit

@kylec32
Copy link
Contributor Author

kylec32 commented Oct 9, 2020

@angeloc Yeah sorry about that. I didn't plan on squashing until all the rest of the items were accounted for as well (testing and fixing the conflicts that now exist) which will probably not be today just figured I would take care of the easy one off the top. I usually just use Github to do the final sqash and merge myself so its just a different process than I usually follow. I'll squash them all together when I'm finished and request your review.

@angeloc
Copy link
Owner

angeloc commented Oct 9, 2020

If you squash the commits on the current branch, then you can do a pull -r origin master and the conflicts should resolve automatically.
Testing anyway should be done in another commit!

Thanks again! Your contribution is very appreciated.

I don't know if you are looking for a tshirt, but I marked your contribution for hacktober fest (https://hacktoberfest.digitalocean.com/)

@pjboro
Copy link

pjboro commented Jan 8, 2021

Hi! Is anyone working on this right now? I can try to take care of this PR, if you do not have time right now @kylec32. It's sensible to prevent restoring the same files - good catch.

@kylec32
Copy link
Contributor Author

kylec32 commented Jan 8, 2021

@pjboro I am a bit at a hiatus of working on this right now due to other work priorities. The code works from my testing, we just need to get some tests for it is all we are really waiting for (and of course fix the conficts which shouldn't be too bad). If you are willing to pick up that work then that is great. No toes will be stepped on here. 👍 (I just looked at the date the above message was sent and realize how silly it is to be responding right now, I'm not sure how I missed your message for so long)

@pjboro
Copy link

pjboro commented Jan 11, 2021

I'm not sure how I missed your message for so long

I believe this is targeted to angeloc? As for my message, 3 days is a quite good response time for an open source project IMHO ;) I'll clone the branch and take a look this week, provided I won't have to take overtime at work.

@Stivens73
Copy link

Posted PR #10. Rebased @kylec32 over master. @angeloc, please, checkout

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants