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

address name contraction issues in argo-workflows create #2082

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

savingoyal
Copy link
Collaborator

remaining items -

  1. enable --name for long names for CLI args except create
  2. check if sanitize_for_argo needs fixing
  3. moar testing
  4. comms

@savingoyal
Copy link
Collaborator Author

closes #1818 , #1608

Copy link
Collaborator

@saikonen saikonen left a comment

Choose a reason for hiding this comment

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

an open question still remains how to handle the other argo CLI commands. f.ex. argo-workflows delete in the case where a deployment with an old name exists. do we try to delete both old&new every time?

Comment on lines +311 to +314
" To comply with new naming restrictions on Argo "
"Workflows, this deployment replaced the\npreviously "
"deployed workflow {v1_workflow_name}.\n".format(
v1_workflow_name=obj._v1_workflow_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could also check whether v1_workflow_name exists or not, and cut this alert out if we did not replace an old deployment

Comment on lines +518 to +519
# TODO: We can create better names that try to preserve the flow name
workflow_name = "%s-%s" % (workflow_name[: limit - 6], name_hash)
Copy link
Collaborator

@saikonen saikonen Oct 7, 2024

Choose a reason for hiding this comment

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

we could use current.flow_name as the base and append the hash which comes from the current.project_flow_name if the flow name is the most significant info in the template name

Though one issue with this would be that validate_run_id relies on checking whether project/branch are part of the generated workflow_name. This needs an overhaul in any case though, as it will most likely start breaking due to the new limits

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.

2 participants