changing display URLs for argo #2016
Conversation
iamsgarg-ob
force-pushed
the
fix-display-urls
branch
2 times, most recently
from
a53025f
to
7e76aaf
Compare
| ARGO_WORKFLOWS_UI_URL.rstrip("/"), | ||
| "{{workflow.namespace}}", | ||
| "{{workflow.name}}", | ||
| "href": ARGO_WORKFLOWS_UI_URL.rstrip("/").format( |
There was a problem hiding this comment.
ARGO_WORKFLOWS_UI_URL should be set as:
https://metaflow.foo.argo.com/{uri}/{namespace}/{suffix}
iamsgarg-ob
force-pushed
the
fix-display-urls
branch
3 times, most recently
from
8f0c18f
to
1c4cc80
Compare
iamsgarg-ob
force-pushed
the
fix-display-urls
branch
from
1c4cc80
to
265d3a4
Compare
| "suffix": obj.workflow_name, | ||
| } | ||
| obj.echo("See the deployed workflow here:", bold=True) | ||
| argo_workflowtemplate_link = ARGO_WORKFLOWS_UI_URL.rstrip("/").format( |
There was a problem hiding this comment.
I assume this was supposed to be part of the above echo? seems to be omitted currently.
| run_url = ARGO_WORKFLOWS_UI_URL.rstrip("/").format( | ||
| **workflow_run_url_values |
There was a problem hiding this comment.
this doesn't raise any exception with "incorrect" url templates. for example
METAFLOW_ARGO_WORKFLOWS_UI_URL=http://example.com/
simply prints out
See the run in the UI at
:
http://example.com
There was a problem hiding this comment.
Yep that is the intended behavior.. the exception gets raised if the template has a variable that is missing from the dictionary that is supplied. If the supplied dictionary has extra vars they will simply be ignored
| "suffix": argo_workflow_id, | ||
| } | ||
| obj.echo(json.dumps(workflow_run_url_values)) | ||
| obj.echo("See the run in the UI at\n:", bold=True) |
| "*{run_url}*".format(run_url=run_url), | ||
| indent=True, | ||
| ) | ||
| except: |
There was a problem hiding this comment.
bare except, add Exception or something more specific
| argo_workflowtemplate_link = ARGO_WORKFLOWS_UI_URL.rstrip("/").format( | ||
| **workflow_template_url_values | ||
| ) | ||
| except: |
There was a problem hiding this comment.
bare except, add Exception or something more specific
| "workflow_name": obj.workflow_name, | ||
| "suffix": argo_workflow_id, | ||
| } | ||
| obj.echo(json.dumps(workflow_run_url_values)) |
There was a problem hiding this comment.
Ha.. good catch.. will remove
| obj.echo( | ||
| "%s/%s\n\n" % (argo_workflowtemplate_link, obj.workflow_name), | ||
| indent=True, | ||
| "workflow template was created - errored in formatting workflow template URL" |
There was a problem hiding this comment.
error message could be more specific that METAFLOW_ARGO_WORKFLOWS_UI_URL could not be used to format the url.
This exception is never raised though, if you try it out with a template url that is missing the required keys
METAFLOW_ARGO_WORKFLOWS_UI_URL=http://example.com/
the only exception you will get is if the template string contains keys that are not present in workflow_template_url_values, such as
METAFLOW_ARGO_WORKFLOWS_UI_URL=http://example.com/{not_present}
There was a problem hiding this comment.
Yes that is the intended behavior. Since it's an existing config var in an effort to be backwards compatible this was a little neat solution
| "%s/%s/%s" % (UI_URL.rstrip("/"), obj.flow.name, run_id) if UI_URL else None | ||
| ) | ||
|
|
||
| if run_url: |
There was a problem hiding this comment.
we do not want to remove the url to Metaflow UI in favour of Argo UI though? preferably we keep both, with tidy messaging as introduced in https://github.com/Netflix/metaflow/pull/1934/files#diff-27da6198697d250e8ed1cea087795ecbbf2a0e8e8e0861dd51c1f0062c2f0c41R653
which is a somewhat overlapping PR to this one.
In this PR I am changing the way the ARGO URLs are constructed. Initially the METAFLOW_ARGO_WORKFLOWS_UI_URL was set to the argo host - and the sub paths were constructed in code.
With this change the METAFLOW_ARGO_WORKFLOWS_UI_URL will be templatized.
The correct template for this setting should be
https://metaflow.argo.com/{uri}/{namespace}/{suffix}The values for the template are threaded through via a dict.
Note: While this change is backwards compatible ( added try/except ) this will cause the argo links in Pagerduty alerts to go wonky. Fix is to change the config and set the value as shown above.