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

SNOW-1734018 SPCS command fixes #1710

Merged
merged 6 commits into from
Oct 17, 2024
Merged

SNOW-1734018 SPCS command fixes #1710

merged 6 commits into from
Oct 17, 2024

Conversation

sfc-gh-sichen
Copy link
Collaborator

@sfc-gh-sichen sfc-gh-sichen commented Oct 13, 2024 •
edited
Loading

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

SNOW-1734018

Updates the following snow spcs commands:
Deprecations

  • Added deprecation warning in the description of snow spcs service status and snow spcs image-repository list-tags.

New additions

  • Added snow spcs service list-instances, snow spcs service list-containers and snow spcs service list-roles commands, which support fetching information about all instances/containers/service roles in a service.

Fixes and improvements

  • snow spcs service set now supports --eai-name to update external access integrations for a service.
  • snow spcs service list-images now displays image tag and digest.

@sfc-gh-sichen sfc-gh-sichen force-pushed the sichen-SNOW-1734018-show-containers branch from 1a33126 to c6e6447 Compare October 13, 2024 21:43
@sfc-gh-sichen sfc-gh-sichen changed the title spcs commands fixes SNOW-1734018 SPCS command fixes Oct 14, 2024
@sfc-gh-sichen sfc-gh-sichen force-pushed the sichen-SNOW-1734018-show-containers branch 4 times, most recently from 1b6fdce to 32b9966 Compare October 14, 2024 18:52
sfc-gh-yiwang
sfc-gh-yiwang previously approved these changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@sfc-gh-yiwang sfc-gh-yiwang left a comment

Choose a reason for hiding this comment

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

LGTM. Please ensure the SPCS integration tests runs successfully before merging.

RELEASE-NOTES.md Outdated

## Fixes and improvements
* `snow spcs service set` now supports `--eai-name` to update external access integrations for a service.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This also looks like a new addition.

RELEASE-NOTES.md Outdated

## Fixes and improvements
* `snow spcs service set` now supports `--eai-name` to update external access integrations for a service.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably also mentioning that we improve the snow spcs service list-images result by including enriched info like tag and sha

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! I will update.

tests/spcs/test_image_repository.py

result = runner.invoke(
["spcs", "image-repository", "list-images", "IMAGES", "--format", "JSON"]
)

assert result.exit_code == 0, result.output
assert json.loads(result.output) == [{"image": "/DB/SCHEMA/IMAGES/super-cool-repo"}]
assert "/db/schema/repo/echo_service" in result.output
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: why don't we assert cursor == result directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The output is in table format. I guess we could make it json and compare. But the return value is faked entirely. For example, it has the same effect as making cursor return foo and assert the result is the same as foo.

tests/spcs/test_services.py

mock_list_instances.assert_called_once_with(service_name=service_name)
assert result.exit_code == 0
assert "TEST_SERVICE" in result.output, str(result.output)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: why don't we assert cursor == result directly?

@sfc-gh-sichen sfc-gh-sichen force-pushed the sichen-SNOW-1734018-show-containers branch from 32b9966 to f3e36a5 Compare October 15, 2024 04:48
@sfc-gh-sichen sfc-gh-sichen marked this pull request as ready for review October 15, 2024 04:48
@sfc-gh-sichen sfc-gh-sichen requested review from a team as code owners October 15, 2024 04:48
sfc-gh-astus
sfc-gh-astus previously approved these changes Oct 15, 2024
View reviewed changes
@sfc-gh-sichen sfc-gh-sichen force-pushed the sichen-SNOW-1734018-show-containers branch 2 times, most recently from 0622a28 to c7dfcb5 Compare October 15, 2024 19:14
@sfc-gh-sichen
Copy link
Collaborator Author

Integration test failed but doesn't look like related
FAILED tests_integration/test_temporary_connection.py::test_temporary_connection - ValueError: Invalid private key

@sfc-gh-sichen sfc-gh-sichen force-pushed the sichen-SNOW-1734018-show-containers branch from c7dfcb5 to b480011 Compare October 16, 2024 16:16
@sfc-gh-sichen sfc-gh-sichen force-pushed the sichen-SNOW-1734018-show-containers branch from af8fb05 to fed0ea1 Compare October 17, 2024 15:39
@sfc-gh-sichen sfc-gh-sichen merged commit ff9f758 into main Oct 17, 2024
19 checks passed
@sfc-gh-sichen sfc-gh-sichen deleted the sichen-SNOW-1734018-show-containers branch October 17, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-astus sfc-gh-astus sfc-gh-astus approved these changes

@sfc-gh-yiwang sfc-gh-yiwang Awaiting requested review from sfc-gh-yiwang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sfc-gh-sichen @sfc-gh-yiwang @sfc-gh-astus