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

NA+SPCS setup script modifications PoC #1827

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

sfc-gh-fcampbell
Copy link
Contributor

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

@sfc-gh-fcampbell sfc-gh-fcampbell force-pushed the frank-spcs-poc branch 2 times, most recently from 655992c to d65286a Compare October 31, 2024 19:21
Comment on lines 603 to 716
return dedent(
f"""\
-- Begin generated SPCS services, this section is managed by the Snowflake CLI
create schema if not exists _spcs_generation;

create or replace procedure {name}(privileges array)
returns string
as $$
begin
{f'call {existing_grant_callback}(privileges);' if existing_grant_callback else ''}
if (array_contains('CREATE COMPUTE POOL'::variant, privileges)) then
create compute pool if not exists {service.compute_pool}
min_nodes = {service.min_nodes}
max_nodes = {service.max_nodes}
instance_family = {service.instance_family};
end if;
if (array_contains('BIND SERVICE ENDPOINT'::variant, privileges)) then
create service if not exists _spcs_generation.{service.fqn.name}
in compute pool {service.compute_pool}
from specification_file = '{service.specification_file}';
end if;
return 'done';
end;
$$;

create application role if not exists _spcs_generation_role;
grant usage on procedure {name}(array) to application role _spcs_generation_role;

-- End generated SPCS services
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you see a way to reuse the same logic from SpcsEntity.deploy() or ServiceEntity.deploy() once it will be implemented?

return bundle_map

def _inject_spcs(self, spcs_services: list[ServiceEntity]):
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks okay as a PoC, but we probably need to be more careful when we auto-modify the manifest and setup script

Comment on lines +120 to +123
services: list[str] = Field(
title="List of Snowpark Container Service entity IDs to integrate into this application package",
default=[],
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure there's a real problem here, but by omitting the proposed children field we lose the ability to order these internal dependencies

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.

2 participants