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

Refactor unreleased commands and hide released spcs flags instead of blocking #1989

Merged
merged 4 commits into from
Jan 14, 2025

Conversation

sfc-gh-pczajka
Copy link
Contributor

@sfc-gh-pczajka sfc-gh-pczajka commented Jan 14, 2025

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

  • Hide --follow and --follow-interval flags if ENABLE_SPCS_LOG_STREAMING flag is disabled
  • Disable snow spcs service metrics/events commands if their feature flags are not set

@sfc-gh-pczajka sfc-gh-pczajka requested a review from a team as a code owner January 14, 2025 12:14
"enable_spcs_service_events = true\n"
"enable_spcs_service_metrics = true\n"
)
yield config_toml
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do this on config file? Mock is_enabled/is_disabled is more obvious and faster. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately - mocking happens after the commands.py file is imported. During import is_enabled function is saved in app factory, which means it is not mocked :/

@sfc-gh-pczajka sfc-gh-pczajka requested a review from a team as a code owner January 14, 2025 12:48
@sfc-gh-pczajka sfc-gh-pczajka changed the title Refactopr unreleased commands and hide released spcs flags instead of blocking Refactor unreleased commands and hide released spcs flags instead of blocking Jan 14, 2025
@sfc-gh-pczajka sfc-gh-pczajka merged commit 39938dc into main Jan 14, 2025
20 checks passed
@sfc-gh-pczajka sfc-gh-pczajka deleted the pczajka-hide-spcs-commands-instead-of-blocking branch January 14, 2025 13:21
sfc-gh-pczajka added a commit that referenced this pull request Jan 14, 2025
…blocking (#1989)

* disable spcs commands if feature flag is disabled; hide logs instead of rising an error

* Hide flag instead of command, fix tests

* update snapshots

* remove streaming flag
This was referenced Jan 14, 2025
sfc-gh-pczajka added a commit that referenced this pull request Jan 14, 2025
* SNOW-1843926: SPCS service events & metrics (#1954)

* spcs service events&metrics

* snapshot and feature flag

* add new test

* fix test

* new table format and new test and new snapshot

* fix comment

* release notes

* fix comments

* snapshot update

* update snapshots

---------

Co-authored-by: Teja Kommineni <teja.kommineni@snowflake.com>
Co-authored-by: Patryk Czajka <patryk.czajka@snowflake.com>

* Refactor unreleased commands and hide released spcs flags instead of blocking (#1989)

* disable spcs commands if feature flag is disabled; hide logs instead of rising an error

* Hide flag instead of command, fix tests

* update snapshots

* remove streaming flag

---------

Co-authored-by: Abby Shen <abby.shen@snowflake.com>
Co-authored-by: Teja Kommineni <teja.kommineni@snowflake.com>
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