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.

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

SNOW-1706990 Extract non action-related fields in ActionContext to new WorkspaceContext #1652

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

sfc-gh-fcampbell
Copy link
Contributor

@sfc-gh-fcampbell sfc-gh-fcampbell commented Oct 1, 2024

Pre-review checklist

  • I've confirmed that instructions included in README.md are still correct after my changes in the codebase.
  • I've added or updated automated unit tests to verify correctness of my new code.
  • I've added or updated integration tests to verify correctness of my new code.
  • I've confirmed that my changes are working by executing CLI's commands manually on MacOS.
  • I've confirmed that my changes are working by executing CLI's commands manually on Windows.
  • I've confirmed that my changes are up-to-date with the target branch.
  • I've described my changes in the release notes.
  • I've described my changes in the section below.

Changes description

Entity classes current have lots of static methods, which were added to support the v1 to v2 migration, but they have way too many parameters, so we want to allow calling these methods on entity instances.

Right now, the only instance methods are EntityAction implementations, which represent the top-level actions that can be performed by these entities, but the static methods we want to convert to instance methods don't need to be EntityAction implementations (since they don't represent top-level operations). However, it would be useful for these methods to have access to utilities in the ActionContext, like ctx.default_role for example. Since these utilities are not really part of the action, let's extract them from ActionContext into a new WorkspaceContext that gets set as a field of the entity when instantiated by the WorkspaceManager.

@sfc-gh-fcampbell sfc-gh-fcampbell force-pushed the frank-action-context-init branch 2 times, most recently from 65bd816 to 828e782 Compare October 2, 2024 14:35
@sfc-gh-fcampbell sfc-gh-fcampbell marked this pull request as ready for review October 2, 2024 15:03
@sfc-gh-fcampbell sfc-gh-fcampbell requested review from a team as code owners October 2, 2024 15:03
@sfc-gh-fcampbell sfc-gh-fcampbell enabled auto-merge (squash) October 2, 2024 15:50
@sfc-gh-fcampbell sfc-gh-fcampbell changed the title Pass context in entity init instead of only to actions SNOW-1706990 Pass context in entity init instead of only to actions Oct 2, 2024
@sfc-gh-bdufour
Copy link
Contributor

Hmm, not sure about this one yet. It made sense to me to decouple the entity model from the action state, but this couples them again. Can you give me specifics about why we want to access things like default_role outside of an action method?

@sfc-gh-fcampbell
Copy link
Contributor Author

I'm trying to get rid of NativeAppManager, NativeAppProjectModel, etc since those classes can only handle the v1 data model, and make the snow app command operate directly on v2 entities. The problem is that these commands don't just call action_ methods on entities, they need supporting methods like get_existing_app_info, get_snowsight_url, etc.

Right now, for an operation like get_existing_app_info, NativeAppManager calls a static method on the ApplicationEntity, passing in the app name and role to use:

    def get_existing_app_info(self) -> Optional[dict]:
        return ApplicationEntity.get_existing_app_info(
            app_name=self.app_name,
            app_role=self.app_role,
        )
    @staticmethod
    def get_existing_app_info(app_name: str, app_role: str) -> Optional[dict]:
        sql_executor = get_sql_executor()
        with sql_executor.use_role(app_role):
            return sql_executor.show_specific_object(
                "applications", app_name, name_col=NAME_COL
            )

In NativeAppManager/NativeAppProjectModel, self.app_name and self.app_role are properties with logic behind them, and if we're to get rid of NativeAppManager/NativeAppProjectModel, that logic needs to move somewhere. Since the same logic already exists for the v2 model in WorkspaceManager/ActionContext, I want to leverage it instead of duplicating it in ApplicationEntity.

The problem I'm facing now is that ActionContext is only available to "implementations" of EntityActions, like action_deploy, action_bundle, etc. Since I don't consider get_existing_app_info to be a "top-level action", it doesn't merit its own entry in EntityActions, meaning it can't be invoked through WorkspaceManager.perform() (which is where the ActionContext is created). I also don't want to set a precedent that any instance methods of entities need to be EntityActions, since that would require a bunch of boilerplate for actions that only one entity type would ever implement.

My solution to this is to set the ActionContext as an instance property of the entity so that any methods can access the current context data. With these changes, get_existing_app_info would become an instance method instead of a static:

    def get_existing_app_info(self) -> Optional[dict]:
        model = self._entity_model
        ctx = self._action_ctx
        role = (model.meta and model.meta.role) or ctx.default_role  #  default_role used here
        sql_executor = get_sql_executor()
        with sql_executor.use_role(role):
            return sql_executor.show_specific_object(
                "applications", model.fqn.name, name_col=NAME_COL
            )

and could be called without needing the whole WorkspaceManager.perform()/EntityActions mechanism:

    app = ws.get_entity(app_id)
    if app.get_existing_app_info():
        typer.launch(app.get_snowsight_url())
        return MessageResult(f"Snowflake Native App opened in browser.")
    else:
        return MessageResult(
            'Snowflake Native App not yet deployed! Please run "snow app run" first.'
        )

@sfc-gh-fcampbell sfc-gh-fcampbell marked this pull request as draft October 2, 2024 19:05
auto-merge was automatically disabled October 2, 2024 19:05

Pull request was converted to draft

@sfc-gh-fcampbell sfc-gh-fcampbell changed the title SNOW-1706990 Pass context in entity init instead of only to actions SNOW-1706990 Extract non action-related fields in ActionContext to new WorkspaceContext Oct 2, 2024
get_entity=self.get_entity,
)
return entity.perform(action, action_ctx, *args, **kwargs)
else:
raise ValueError(f'This entity type does not support "{action.value}"')

@property
Copy link
Contributor Author

@sfc-gh-fcampbell sfc-gh-fcampbell Oct 2, 2024

Choose a reason for hiding this comment

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

drive-by improvement, we never take a reference to this method so it can be a property instead, like it is in other classes

@sfc-gh-fcampbell sfc-gh-fcampbell force-pushed the frank-action-context-init branch 2 times, most recently from e685a5e to 04f8641 Compare October 2, 2024 20:07
@sfc-gh-fcampbell sfc-gh-fcampbell marked this pull request as ready for review October 2, 2024 20:39
@sfc-gh-fcampbell sfc-gh-fcampbell enabled auto-merge (squash) October 2, 2024 20:39
@sfc-gh-fcampbell
Copy link
Contributor Author

Description of changes after that long post above. A previous version of this PR was passing the whole ActionContext to the entity's __init__, but now we've decided to split that class into WorkspaceContext to give access to workspace-wide config, like the console, default role, etc, and keep only get_entity in ActionContext.

sfc-gh-pjafari
sfc-gh-pjafari previously approved these changes Oct 3, 2024
Copy link
Contributor

@sfc-gh-pjafari sfc-gh-pjafari left a comment

Choose a reason for hiding this comment

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

I like this - the first time I read ActionContext I was wondering why things like get_default_role and get_default_warehouse are action context. This makes more sense :)

"""
An object that is passed to each action when called by WorkspaceManager
An object that is passed to each entity when instantiated by WorkspaceManager
to allow access to the CLI context without required the entities to use
Copy link
Contributor

Choose a reason for hiding this comment

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

"without requiring"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

…ntext, which is stored as instance field on entities
@sfc-gh-fcampbell sfc-gh-fcampbell enabled auto-merge (squash) October 3, 2024 15:54
@sfc-gh-fcampbell sfc-gh-fcampbell merged commit 7e3b9fb into main Oct 3, 2024
19 checks passed
@sfc-gh-fcampbell sfc-gh-fcampbell deleted the frank-action-context-init branch October 3, 2024 16:24
sfc-gh-sichen pushed a commit that referenced this pull request Oct 17, 2024
…w WorkspaceContext (#1652)

Entity classes current have lots of static methods, which were added to support the v1 to v2 migration, but they have way too many parameters, so we want to allow calling these methods on entity instances. 

Right now, the only instance methods are `EntityAction` implementations, which represent the top-level actions that can be performed by these entities, but the static methods we want to convert to instance methods don't need to be `EntityAction` implementations (since they don't represent top-level operations). However, it would be useful for these methods to have access to utilities in the `ActionContext`, like `ctx.default_role` for example. Since these utilities are not really part of the action, let's extract them from `ActionContext` into a new `WorkspaceContext` that gets set as a field of the entity when instantiated by the `WorkspaceManager`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants