Use PluginCleanupPolicy from flyteorg/flyteplugins#203 #311
Conversation
|
Thank you for opening this pull request! 🙌
|
clairemcginty
force-pushed
the
use_cleanup_policy
branch
from
908bf5a to
6ba0161
Compare
| } else if behavior.Update != nil { | ||
| err = e.kubeClient.GetClient().Update(ctx, o, behavior.Update.Options...) | ||
| } else { | ||
| err = errors.Errorf(errors.RuntimeFailure, "AbortBehavior for resource %v must specify either a Patch and an Update operation if Delete is set to false. Only one can be supplied.", o.GetName()) |
There was a problem hiding this comment.
Maybe worth adding a custom error type to flyteplugins' errors package? InvalidPluginConfiguration or something like that? cc @EngHabu
|
looks good to me, I will let @EngHabu +1. |
clairemcginty
force-pushed
the
use_cleanup_policy
branch
from
8fd1cf0 to
1c7a8dd
Compare
|
thanks @EngHabu, I applied your suggestions! I also updated the flyteplugins version (+ dependencies) ; looks like I might not have permissions to modify go.mod/go.sum files though...? |
| assert.Equal(t, expectedErr, err) | ||
| }) | ||
|
|
||
| t.Run("Abort Plugin has Delete PluginAbortOverride", func(t *testing.T) { |
There was a problem hiding this comment.
I didn't include tests for AbortBehaviors with a non-nil Resource because the existing test framework made it a little difficult (can't make assertions on the calls made to the extendedFakeClient as it's not a mock)... I'll brainstorm a bit if there's an easier way to test that.
|
You will need to run |
Signed-off-by: Claire McGinty <[email protected]>
Signed-off-by: Claire McGinty <[email protected]>
Signed-off-by: Claire McGinty <[email protected]>
Signed-off-by: Claire McGinty <[email protected]>
Signed-off-by: Claire McGinty <[email protected]>
Signed-off-by: Claire McGinty <[email protected]>
Signed-off-by: Claire McGinty <[email protected]>
Signed-off-by: Claire McGinty <[email protected]>
clairemcginty
force-pushed
the
use_cleanup_policy
branch
from
1c7a8dd to
ca2ba22
Compare
|
🙇🏽 |
EngHabu
changed the title
|
Congrats on merging your first pull request! 🎉 |
* use PluginCleanupPolicy if it exists Signed-off-by: Claire McGinty <[email protected]> * add unit test Signed-off-by: Claire McGinty <[email protected]> * cleanup test code Signed-off-by: Claire McGinty <[email protected]> * assert OnAbort is not attempted by default Signed-off-by: Claire McGinty <[email protected]> * update plugin override interface Signed-off-by: Claire McGinty <[email protected]> * lint Signed-off-by: Claire McGinty <[email protected]> * Apply PR suggestions Signed-off-by: Claire McGinty <[email protected]> * update flyteplugins lib Signed-off-by: Claire McGinty <[email protected]>
TL;DR
Per discussion in
#feature-discussionsSlack channel, I'm opening this as a draft PR as to center discussions related to flyteorg/flyte#1345.This PR applies the
PluginCleanupPolicyinterface from flyteorg/flyteplugins#203, if applicable, instead of defaulting to just deleting the resource when the task is aborted.this will need to be updated once the flyteplugins changed is merged & its version is bumped in
go.mod.cc @regadas , @EngHabu , @kumare3 :)
Type
Are all requirements met?
Complete description
How did you fix the bug, make the feature etc. Link to any design docs etc
Tracking Issue
Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue
fixes https://github.com/flyteorg/flyte/issues/
Follow-up issue
NA
OR
https://github.com/flyteorg/flyte/issues/