Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Make argo events sensor namespace configurable #1463
base: master
Are you sure you want to change the base?
Make argo events sensor namespace configurable #1463
Changes from 13 commits
8cffd98
57c1f62
3510429
6372e15
b2071a3
b444768
f7406b9
0899a45
bab09bf
18681bf
f29c08d
6f3ece8
4d9bb49
dc00696
5609270
a680b66
4115a44
348c0b9
85785ee
333ee5c
39284f2
6df2c25
89bf94a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I believe there is an issue with running the cleanup only as part of the deploy command. Consider the following scenario:
dev-A
, sensor exists in the same namespacedev-B
argo-workflows delete
directly.Wouldn't this only check for the currently configured sensor namespace, leaving the original sensor in
dev-A
untouched?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.
At least in the case of
delete
it seems sensible to me to keep the concern of cleaning up resources as part of the method.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.
For the legacy case, we need to do cleanup before creating a new argo workflow. This can only happen if we clean up before calling
flow.deploy()
.One reason I did this is that there is some inconsistency in the API. The current implementation already does some sort of "cleanup". For schedules, we create/suspend them within the
schedule_workflow_template
method which is in theargo_client.py
, which is called within theschedule
method inargo_workflows.py
. However the sensors are created/deleted within theschedule
method inargo_workflows.py
. So I wanted to move this all into one place, but I couldn't put my code into thisschedule
method. The previous annotations for the triggers would be overwritten before theschedule
method.Currently, the
flow.schedule
function is used to create both the schedule and the sensor with the assumption that the sensor will get overwritten because it always exists in the same namespace. So in your scenario, if you configured you local environment withdev-B
then you would have to do anargo-workflows create
in order to create a sensor indev-B
. Once you did that, it would delete the sensor indev-A
due to this line:https://github.com/dhpollack/metaflow/blob/feature/configure-sensor-namespace/metaflow/plugins/argo/argo_workflows.py#L195