Use placeholder templates for registration params #316
Conversation
katrogan
requested review from
kumare3,
matthewphsmith and
wild-endeavor
as code owners
flytekit/clis/helpers.py
Outdated
| identifier.project = identifier.project or project | ||
| identifier.domain = identifier.domain or domain | ||
| identifier.version = identifier.version or version | ||
| if not identifier.project or identifier.project == "{{ .project }}": |
There was a problem hiding this comment.
should these not be regexes?
There was a problem hiding this comment.
if you want example regex look at https://github.com/lyft/flytekit/blob/annotations/flytekit/annotated/base_sql_task.py#L15
There was a problem hiding this comment.
perhaps, but in the case of the project field the only templatized variable that can ever be used is {{ .project }}
There was a problem hiding this comment.
ya but what about whitespaces?
There was a problem hiding this comment.
also this should be changed right?
There was a problem hiding this comment.
derp, yes. had my test case backwards 🤦
There was a problem hiding this comment.
re: whitespace, the templating only ever comes from flytesdk right? presumably we can trust that to properly assign the constants, whitespace and all?
|
auth role and raw output data prefix? |
we use the config values at serialization time. |
|
no preference on |
|
I like the idea of |
they shouldnt be at serialization right? actually tbh, they are just Admin attributes so should probably do not even exist at serialization time? |
|
@kumare3 yep eventually the goal to make admin handle them flyteorg/flytectl#18 i'll update |
|
PTAL |
flytekit/clis/flyte_cli/main.py
Outdated
| "--kubernetes-service-account", help="Custom kubernetes service account auth role to register launch plans with" | ||
| "-s", | ||
| "--kubernetes-service-account", | ||
| help="Custom kubernetes service account auth role to register launch plans with", | ||
| ) | ||
| _output_location_prefix_option = _click.option( | ||
| "--output-location-prefix", help="Custom output location prefix for offloaded types (files/schemas)" |
There was a problem hiding this comment.
Can we add a short form for this too?
| _PROJECT_PLACEHOLDER = "" | ||
| _DOMAIN_PLACEHOLDER = "" | ||
| _VERSION_PLACEHOLDER = "" | ||
| _PROJECT_PLACEHOLDER = "{{ registration.project }}" |
There was a problem hiding this comment.
Can you summarize why there aren't placeholders for auth and output prefix?
There was a problem hiding this comment.
neither are required during serialization
besides auth is a oneof - what should i have a placeholder for: the k8s service account or assumable iam role?
|
friendly ping |
|
ping @wild-endeavor @kumare3 |
|
lgtm, I will wait for @wild-endeavor to +1 |
|
approved offline |
No description provided.