-
Notifications
You must be signed in to change notification settings - Fork 58
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
Project definition v2 entity schemas: application and application package #1280
Conversation
7cafab1
to
0eaf420
Compare
7f38deb
to
244483a
Compare
) | ||
|
||
|
||
class ApplicationFromField(UpdatableModel): |
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.
(Non-blocking) In the future, we should pull the generic type attr up, e.g.
from_: EntityReferenceField[ApplicationPackageEntity]
src/snowflake/cli/api/project/schemas/entities/application_entity.py
Outdated
Show resolved
Hide resolved
) | ||
|
||
|
||
class EntityBase(ABC, UpdatableModel): |
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.
(Non-blocking) I would like to see e.g. StageBasedEntity
, DatabaseLevelEntity
, SchemaLevelEntity
, AccountLevelEntity
in the future to maximize re-use and align with the func spec
_v2_entity_types_map = { | ||
EntityType.APPLICATION.value: ApplicationEntity, | ||
EntityType.APPLICATION_PACKAGE.value: ApplicationPackageEntity, | ||
} |
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.
Next up for us is to figure out how these pydantic models map to classes with e.g. deploy, drop, bundle methods
from snowflake.cli.api.project.schemas.native_app.path_mapping import PathMapping | ||
|
||
|
||
class ApplicationPackageEntity(EntityBase): |
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.
There are some properties not accounted for here, like scratch_stage
, generated_root
, and bundle_root
. What's the plan for those?
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.
Added these as well. While we maintain both versions we will have to remember to update v2 schema when we make v1 changes.
src/snowflake/cli/api/project/schemas/entities/application_entity.py
Outdated
Show resolved
Hide resolved
title="Distribution of the application package created by the Snowflake CLI", | ||
default="internal", | ||
) | ||
manifest: str = Field( |
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.
manifest: str = Field( | |
manifest: Path = Field( |
Or SecurePath
.
class EntityType(Enum): | ||
APPLICATION = "application" | ||
APPLICATION_PACKAGE = "application package" |
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.
We have those strings as literal on entities classes and we have them here. Should we consider using this Enum as source of truth? Or should we use entities to build Enum / something?
Taking into account that we are constructing Union[ApplicationEntity, ApplicationPackageEntity]
and
_v2_entity_types_map = {
EntityType.APPLICATION.value: ApplicationEntity,
EntityType.APPLICATION_PACKAGE.value: ApplicationPackageEntity,
}
It means that adding new entity requires it to be registered in at least two places.
it may be wise to have
KNOWN_ENTITIES = [
ApplicationEntity,
ApplicationPackageEntity
]
ENTITIES_TYPE = Union[*KNOWN_ENTITIES]
_v2_entity_types_map = {get_args(eval(inspect.get_annotations(e)["type"]))[0]: e for e in KNOWN_ENTITIES}
The dict comprehension is a bit of a magic, but probably there's something smarter we can do.
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.
Yes, that's what I tried at first but I couldn't make pydantic understand enums as discriminators and reverted to literal strings.
I'm not sure what I think about this dict comprehension, but I get the idea. I'll try to come up with a cleaner way to have a single source of truth.
title="Role to use when creating the entity object", | ||
default=None, | ||
) | ||
post_deploy: Optional[List[ApplicationPostDeployHook]] = Field( |
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.
Is this common for all entities?
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.
Yep
if "defaults" in data: | ||
if "entities" in data: |
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.
if "defaults" in data and "entities" in data:
|
||
ALL_ENTITIES = [*get_args(Entity)] | ||
|
||
v2_entity_types_map = {e.get_type(): e for e in ALL_ENTITIES} |
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.
Nice!
# TODO Automatically detect TargetFields to validate | ||
if entity.type == ApplicationEntity.get_type(): | ||
if isinstance(entity.from_.target, TargetField): | ||
target = str(entity.from_.target) | ||
if target not in entities: | ||
raise ValueError(f"No such target: {target}") | ||
else: | ||
# Validate the target type | ||
target_cls = entity.from_.__class__.model_fields["target"] | ||
target_type = target_cls.annotation.__args__[0] | ||
actual_target_type = entities[target].__class__ | ||
if target_type and target_type is not actual_target_type: | ||
raise ValueError( | ||
f"Target type mismatch. Expected {target_type.__name__}, got {actual_target_type.__name__}" |
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.
I imagine that there will more checks on the enitites should we consider something like
# TODO Automatically detect TargetFields to validate | |
if entity.type == ApplicationEntity.get_type(): | |
if isinstance(entity.from_.target, TargetField): | |
target = str(entity.from_.target) | |
if target not in entities: | |
raise ValueError(f"No such target: {target}") | |
else: | |
# Validate the target type | |
target_cls = entity.from_.__class__.model_fields["target"] | |
target_type = target_cls.annotation.__args__[0] | |
actual_target_type = entities[target].__class__ | |
if target_type and target_type is not actual_target_type: | |
raise ValueError( | |
f"Target type mismatch. Expected {target_type.__name__}, got {actual_target_type.__name__}" | |
# TODO Automatically detect TargetFields to validate | |
if entity.type == ApplicationEntity.get_type(): | |
self._validate_application_targets(entity) |
WDYT?
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.
I split it into 2 methods to have slightly better separation, but we will need to refactor deeper when we find a generic way for this validation. Will address it in a dedicated PR.
) | ||
generated_root: Optional[Path] = Field( | ||
title="Subdirectory of the deploy root where files generated by the Snowflake CLI will be written.", | ||
default="__generated/", |
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.
Should default be a Path or conversion happens automagicaly?
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.
Automagically, but made it Path
here too to make it clearer
…kage (#1280) * application and application package entity schemas * add integration test * simplify entity type, use discriminator * create types map dynamically * get_type method * read entity list from union
Pre-review checklist
Changes description
The changes in this PR are a step towards enabling
definition_version: 2
insnowflake.yml
. The changes are behind the hiddensnow ws validate
command, which requires enabling theenable_project_definition_v2
feature flag.Changed the v2 schema to have a proper
entities
field that validates the type of each entity and instantiates the matching Pydantic entity model:And added schemas for the
ApplicationPackageEntity
andApplicationEntity
entities.Infra features:
EntityBase
class for all entities to extendMetaField
DefaultsField
will be added if the field exists on the schema class and not specified by the userTargetField
takes a generic entity type, which is verified as part of the schema validationTest
Two manual tests that were not simple to automate:
TargetField
is enforced. ChangedApplicationEntity.from.target
to match a different type:And verified that
snow ws validate
fails:Added
print(cli_context.project_definition)
to the validate command and verified the value ofname
ismy_app_pkg_bar
: