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

Add --only-output-json-directory Argo Workflow option #1947

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

trhodeos
Copy link
Contributor

@trhodeos trhodeos commented Aug 8, 2024

This supports outputting all k8s templates into stdout (rather than just the workflowtemplate one via --only-json).

Fixes #1940, #1730

setup.py Outdated Show resolved Hide resolved
metaflow/plugins/argo/argo_workflows_cli.py Outdated Show resolved Hide resolved
metaflow/plugins/argo/argo_workflows_cli.py Outdated Show resolved Hide resolved
@trhodeos trhodeos requested a review from savingoyal August 8, 2024 17:37
@trhodeos trhodeos changed the title Add --only-yaml argo-workflows cli option. Add --only-json-directory argo-workflows cli option. Aug 13, 2024
@trhodeos trhodeos changed the title Add --only-json-directory argo-workflows cli option. Add --only-cron-workflow-template-only, --only-event-sources-only, and --only-workflow-template-only cli options Aug 22, 2024
@trhodeos trhodeos changed the title Add --only-cron-workflow-template-only, --only-event-sources-only, and --only-workflow-template-only cli options Add --only-cron-workflow-only, --only-event-sources-only, and --only-workflow-template-only cli options Aug 22, 2024
@trhodeos trhodeos changed the title Add --only-cron-workflow-only, --only-event-sources-only, and --only-workflow-template-only cli options Add --only-cron-workflow-json, --only-event-sources-json, and --only-workflow-template-json cli options Aug 22, 2024
@trhodeos trhodeos changed the title Add --only-cron-workflow-json, --only-event-sources-json, and --only-workflow-template-json cli options Add --only-cron-workflow-json, --only-event-sources-json, and --only-workflow-template-json Argo Workflow options Aug 22, 2024
@trhodeos
Copy link
Contributor Author

@savingoyal any chance you can take another look? Thanks in advance!

@trhodeos
Copy link
Contributor Author

trhodeos commented Sep 4, 2024

@savingoyal friendly ping.. Would I be able to get another review?

@@ -3754,8 +3806,7 @@ class ArgoWorkflowTrigger(object):
# https://github.com/argoproj/argo-events/blob/master/api/sensor.md#argoproj.io/v1alpha1.ArgoWorkflowTrigger

def __init__(self):
tree = lambda: defaultdict(tree)
self.payload = tree()
self.payload = _tree()
Copy link
Collaborator

Choose a reason for hiding this comment

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

this change seems unrelated to the PR. can you consider pulling this out as a different PR or skipping it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, let me revert

@@ -2731,7 +2745,7 @@ def _compile_sensor(self):
)

# Useful to paint the UI
trigger_annotations = {
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

@trhodeos we have impending plans to move away from one sensor -> one workflow template mapping (to save resource costs) to one sensor -> multiple workflow templates which will break the expectations in this PR for --only-event-sensor-json. can you help us with the timelines you are shooting for this PR? happy to chat over zoom if that's helpful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

depending on your use case, an immediate workaround could also be to override argo_client.py through the extensions such that rather than submitting the CRDs to kubernetes, it just prints them out to stderr.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but regardless this change is in the right direction. can you help us with details on how you have tested this change? we should be able to test this PR ourselves some point next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One potential solution is to only add a CLI arg --output-json-directory-only and just push all json blobs to that directory. This would essentially allow us to not be tied to the number of resources we generate (as in, adding more sensors is not a problem).

I like the argo_client.py idea, though we'd still like to push directly to argo while we migrate off of using metaflow directly. Still possible, but may require a bit more thinking.

For context: we are trying to use metaflow to generate the resources, spit them out to a gitops repo which has ArgoCD hooked up to it, for better staging of deploys.

As for testing, I'll test locally tomorrow!

@trhodeos trhodeos changed the title Add --only-cron-workflow-json, --only-event-sources-json, and --only-workflow-template-json Argo Workflow options Add --only-output-json-directory Argo Workflow option Sep 19, 2024
@trhodeos trhodeos requested a review from savingoyal September 19, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

argo-workflows create --only-json doesn't export sensor configuration
2 participants