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.

Sign up for GitHub

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

Jump to bottom

Use placeholder templates for registration params #316

Merged
merged 5 commits into from
Jan 11, 2021
Merged

Use placeholder templates for registration params #316

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e657e96
templates
katrogan Jan 6, 2021
079c82c
review comments
katrogan Jan 7, 2021
3520787
fix
katrogan Jan 7, 2021
d8c7cec
short form
katrogan Jan 7, 2021
82c1f9f
comment
katrogan Jan 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions flytekit/clis/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,14 @@ def str2bool(str):
def _hydrate_identifier(
project: str, domain: str, version: str, identifier: _identifier_pb2.Identifier
) -> _identifier_pb2.Identifier:
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 }}":
Copy link
Contributor

Choose a reason for hiding this comment

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

should these not be regexes?

Copy link
Contributor

Choose a reason for hiding this comment

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

if you want example regex look at https://github.com/lyft/flytekit/blob/annotations/flytekit/annotated/base_sql_task.py#L15

Copy link
Contributor Author

@katrogan katrogan Jan 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

perhaps, but in the case of the project field the only templatized variable that can ever be used is {{ .project }}

Copy link
Contributor

Choose a reason for hiding this comment

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

ya but what about whitespaces?

Copy link
Contributor

Choose a reason for hiding this comment

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

also this should be changed right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

derp, yes. had my test case backwards 🤦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: whitespace, the templating only ever comes from flytesdk right? presumably we can trust that to properly assign the constants, whitespace and all?

identifier.project = project

if not identifier.domain or identifier.domain == "{{ .domain }}":
identifier.domain = domain

if not identifier.version or identifier.version == "{{ .version }}":
identifier.version = version
return identifier


Expand Down
6 changes: 3 additions & 3 deletions flytekit/clis/sdk_in_container/serialize.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@
from flytekit.tools.fast_registration import filter_tar_file_fn as _filter_tar_file_fn
from flytekit.tools.module_loader import iterate_registerable_entities_in_order

_PROJECT_PLACEHOLDER = ""
_DOMAIN_PLACEHOLDER = ""
_VERSION_PLACEHOLDER = ""
_PROJECT_PLACEHOLDER = "{{ .project }}"
_DOMAIN_PLACEHOLDER = "{{ .domain }}"
_VERSION_PLACEHOLDER = "{{ .version }}"


class SerializationMode(_Enum):
Expand Down
10 changes: 10 additions & 0 deletions tests/flytekit/unit/cli/test_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,16 @@ def test_hydrate_identifier():
assert identifier.domain == "domain"
assert identifier.version == "12345"

identifier = _hydrate_identifier(
"{{ .project }}",
"{{ .domain }}",
"{{ .version }} ",
_identifier_pb2.Identifier(project="project", domain="domain", version="12345"),
)
assert identifier.project == "project"
assert identifier.domain == "domain"
assert identifier.version == "12345"


def test_hydrate_workflow_template():
workflow_template = _core_workflow_pb2.WorkflowTemplate()
Expand Down