Add copy all options to register script #2464
Conversation
bgedik
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy and
samhita-alla
as code owners
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
Signed-off-by: bugra.gedik <[email protected]>
Signed-off-by: bugra.gedik <[email protected]>
Signed-off-by: bugra.gedik <[email protected]>
… github.com:bgedik/flytekit into bugra.gedik/add_copy_all_options_to_register_script
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2464 +/- ##
===========================================
+ Coverage 41.82% 78.44% +36.62%
===========================================
Files 182 182
Lines 18505 18514 +9
Branches 3856 3643 -213
===========================================
+ Hits 7740 14524 +6784
+ Misses 10637 3331 -7306
- Partials 128 659 +531 ☔ View full report in Codecov by Sentry. |
flytekit/tools/fast_registration.py
Outdated
| FastPackageOptions is used to set configuration options when packaging files. | ||
| """ | ||
|
|
||
| ignores: list[Ignore] = None |
There was a problem hiding this comment.
| ignores: list[Ignore] = None | |
| ignores: Optional[list[Ignore]] = None |
There was a problem hiding this comment.
My bad, this was supposed to be a mandatory one, removed the None
flytekit/remote/remote.py
Outdated
| @@ -965,7 +970,7 @@ def register_script( | |||
| project: typing.Optional[str] = None, | |||
| domain: typing.Optional[str] = None, | |||
| destination_dir: str = ".", | |||
| copy_all: bool = False, | |||
| copy_all: typing.Union[bool, FastPackageOptions] = False, | |||
There was a problem hiding this comment.
can we make this a separate arg? it can be ignored if copy is not used.
|
thanks @bgedik - the copy_all union though, could we split that out please? I think it's nicer to have that explicitly split out. |
|
test error looks like it's not related, but take a look at lint? |
…copy_all_options_to_register_script
@wild-endeavor The lint is fixed, but there is now an unrelated error from Mac build |
|
@bgedik not sure what is happening with the test failure, but it's been fixed on master. Can you pull in master and push to this pr please? |
…k/add_copy_all_options_to_register_script
|
Congrats on merging your first pull request! 🎉 |
Signed-off-by: bugra.gedik <[email protected]> Signed-off-by: bugra.gedik <[email protected]>
Signed-off-by: bugra.gedik <[email protected]> Signed-off-by: Jan Fiedler <[email protected]>
Why are the changes needed?
FlyteRemote.register_script()copies the entire source directory with some default ignore filters, but does not let the user customize the ignore filters.What changes were proposed in this pull request?
API changes to allow additional ignore filters during workflow registery.
How was this patch tested?
Unit tests for
fast_registration.pySetup process
Screenshots
Check all the applicable boxes
Related PRs
?
Docs link
?