Add feature flag for enable_release_channels #1910
Conversation
|
|
||
|
|
||
| def get_ui_parameter( | ||
| conn: SnowflakeConnection, parameter: UIParameter, default: Any | ||
| ) -> str: | ||
| conn: SnowflakeConnection, parameter: UIParameter, default: T |
There was a problem hiding this comment.
this is an odd signature. If we find a value, then it's for sure a string. If we fall back to the default, it could be something else. Shouldn't the default be something like Optional[str] instead?
There was a problem hiding this comment.
I tried it - it doesn't help much because type checker will complain about None not having .lower() function. This one works nicely for typechecking.
i.e. since the return type is str | T, the type checker is able to smartly reason about it based on the passed in default.
| @@ -155,5 +155,5 @@ def _should_invoke_processors(self): | |||
|
|
|||
| def _is_enabled(self, processor: ProcessorMapping) -> bool: | |||
| if processor.name.lower() == NA_SETUP_PROCESSOR: | |||
| return FeatureFlag.ENABLE_NATIVE_APP_PYTHON_SETUP.is_enabled() | |||
| return FeatureFlag.ENABLE_NATIVE_APP_PYTHON_SETUP.get_flag_value() is True | |||
There was a problem hiding this comment.
couldn't we alias is_enabled to this so that we can use either kind of feature flag without worrying about the difference?
There was a problem hiding this comment.
How do you suggest doing this?
- is_enabled: True if set to true, False if set to False, None otherwise?
- is_disabled: True if set to false, False if set to True, None otherwise?
OR:
- is_enabled: True if set to true, False otherwise (including None).
- is_disabled: False if set to false, False otherwise (including None).
Optional: - is_set: True if explicitly set.
WDYT?
There was a problem hiding this comment.
I was thinking: is_enabled(default: False), so by default, the 2nd option you listed, but the first one can be retrofitted.
| "ENABLE_NATIVE_APP_PYTHON_SETUP", False | ||
| ) | ||
| ENABLE_RELEASE_CHANNELS = OptionalBooleanFlag("ENABLE_RELEASE_CHANNELS", None) |
There was a problem hiding this comment.
heads up: you and Guy will have conflicts here
|
|
||
| if feature_flag_from_config is not None and not feature_enabled_in_account: | ||
| self._workspace_ctx.console.warning( | ||
| f"Cannot use feature flag {FeatureFlag.ENABLE_RELEASE_CHANNELS.name} because release channels are not enabled in the current account." |
There was a problem hiding this comment.
this is a warning, but the wording suggests an error. I'd prefer to word it so that the user knows the feature flag will be essentially ignored.
sfc-gh-melnacouzi
force-pushed
the
melnacouzi-add-release-channel-feature-flag
branch
from
f7a47f8 to
fce7d76
Compare
src/snowflake/cli/api/config.py
Outdated
| @@ -286,8 +286,12 @@ def get_config_value(*path, key: str, default: Optional[Any] = Empty) -> Any: | |||
| raise | |||
|
|
|||
|
|
|||
| def get_config_bool_value(*path, key: str, default: Optional[Any] = Empty) -> bool: | |||
| value = get_config_value(*path, key=key, default=default) | |||
| def get_config_bool_value(*path, key: str, default: Any = Empty) -> bool | None: | |||
There was a problem hiding this comment.
that signature doesn't make sense to me, looks like we can return Optional[Any] with the current logic. That wasn't the case before because we tried to cast any value to a bool.
Do we really need Any anyway? Optional[bool] is probably a lot more intuitive.
sfc-gh-melnacouzi
force-pushed
the
melnacouzi-add-release-channel-feature-flag
branch
from
b9c8c05 to
c393435
Compare
sfc-gh-melnacouzi
deleted the
melnacouzi-add-release-channel-feature-flag
branch
Pre-review checklist
Changes description