-
Notifications
You must be signed in to change notification settings - Fork 671
feat: add remote import s3 #2357
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
Conversation
@max-wittig @nejch can you review this PR please? Pretty similar to #2348 |
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.
Looks great @abhiandthetruth thanks for the quick work again. Just a few quick notes :)
There is again a bit of replicated code from import_project function. Can't find a way to refactor it because it is quite small and creating any common entity just feals like overkill. Let me know if anyone has a way to go over it, else I think this much replication might be tolerable.
I agree, no need yet. We have plenty of other custom methods where this is an issue as well.
I think it would need a lot of rework but another decorator with required/optional payload attributes might make these methods more declarative instead of parsing args inside every method. Something like #1999 if we revive that at some point.
@nejch implemented the suggested changes. |
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.
Thanks a lot for this @abhiandthetruth, as you seem keen on contributing more later I added a small non-blocking note for a potential follow-up :) LGTM!
Closes #2356