-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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 extra link work in UI #25500
Make extra link work in UI #25500
Conversation
return render_template_as_native(template, context) | ||
return render_template_to_string(template, context) | ||
return render_template_as_native(template, cast(MutableMapping[str, Any], context)) | ||
return render_template_to_string(template, cast(MutableMapping[str, Any], context)) |
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.
Unrelated, but Mypy is complaining. context
here is implemented as a MutableMapping (although we supply a type stub to say it’s not to prevent users from getting a wrong idea), so this is fine.
bbf31ef
to
72d55bd
Compare
This has two parts: First, the existing extra links mechanism relies on the operator being unmapped, so we do that for most operators to keep working. Second, the UI needs to be slightly updated to ask for those links at the right time -- namely, it must only make requests against unmapped tasks, not the mapped ones.
72d55bd
to
4a6632e
Compare
Hmmm. Ideally It would be nice if each individual mapped task could have it's own mapped links too. For example if your mapped task submits jobs to EMR/Databricks etc then being able to have mapped links to the remote logs/job is desirable. |
Yes, this is what this PR intends to do. What we’re currently doing is completely the other way around: we display the links against the entire mapped task (and that doesn’t work well for most things), but not for individual (mapped) task instances. This PR removes the former, and adds the links for the latter. |
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.
Fantastic ! Thnks @uranusjr for looking at that :)
Close #25360. Two parts in this: