-
Notifications
You must be signed in to change notification settings - Fork 427
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
feat: Add missing session parameters #2936
Conversation
Integration tests cancelled for f17f616fd3979e07e3165e097a4a6e315c9efa36 |
Integration tests failure for 7e27aaef906fa176c592131aef14f3002cf7469f |
sessionParameters.LockTimeout = Pointer(v) | ||
err = setIntegerValue(parameter, value, &sessionParameters.LockTimeout) | ||
case SessionParameterLogLevel: | ||
sessionParameters.LogLevel = Pointer(LogLevel(value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For enum do we want to use ToX(...) functions for enums? If yes, then ToLogLevel is available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about this and was worried about possible breaking changes. Using a bad value should end with an error either way, but sometimes, values that we do not accept are accepted in Snowflake (so this is a part I was worried about). These parameters are used in multiple resources, so it's a bit of a waste to now try to announce this potential change in each of them. We can for sure do it with the redesign, because we are listing the possible values then.
Would you be okay with the TODO for session_parameters issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the TODO would be ok. I was thinking about something similar to a configuration list (just a slice of structs) that would generate appropriate mappings (for a future improvement ofc) etc where it could look similar to
var parameters = {
"param_name": {Levels: []Level{Account, Session, User}, Type: TypeInt, ....}
}
# Conflicts: # v1-preparations/ESSENTIAL_GA_OBJECTS.MD
Integration tests failure for 5bd0bcd3d4919a906dbc8720817ee20fe04b0d82 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about other session parameters like CLIENT_ENABLE_LOG_INFO_STATEMENT_PARAMETERS
, ENABLE_UNHANDLED_EXCEPTIONS_REPORTING
, JDBC_ENABLE_PUT_GET
, JS_TREAT_INTEGER_AS_BIGINT
?
@@ -199,85 +173,123 @@ func GetSessionParametersUnsetFrom(params map[string]any) (*SessionParametersUns | |||
} | |||
|
|||
func (sessionParametersUnset *SessionParametersUnset) setParam(parameter SessionParameter) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I know this is not a part of this PR, but shouldn't the name be unsetParam
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm, I think both are valid: it's setting the given param on the unset object, so this is setting a param after all :)
Prepare user SDK for the user resource/datasource rework (continuation of #2936): - add missing user parameters - fix typos - add missing integration tests (default creation, creation with all properties and parameters, altering, rename, tags) - change a few types (mostly identifiers to the DefaultX values) - document not working parts - add snowflake objects assertions (manually) What's not a part of this PR: - new type of assertions could be added (assertion for parameters list) - additions in 8.26 version of Snowflake (the docs were added today, so it will be a separate change) - setting/unsetting different policies
🤖 I have created a release *beep* *boop* --- ## [0.94.0](v0.93.0...v0.94.0) (2024-07-26) ### 🎉 **What's new:** * Add missing session parameters ([#2936](#2936)) ([4ce662d](4ce662d)) * Adjust user SDK ([#2947](#2947)) ([1127bb3](1127bb3)) * Better tests poc ([#2917](#2917)) ([ef496c2](ef496c2)) * Introduce assertions generators part1 ([#2952](#2952)) ([1582a9f](1582a9f)) * Introduce assertions generators part2 ([#2956](#2956)) ([f715e8a](f715e8a)) * network policy v1 readiness ([#2914](#2914)) ([3408c3f](3408c3f)) * Rework schema datasource ([#2954](#2954)) ([f70e40e](f70e40e)) * Rework schema resource ([#2955](#2955)) ([400a5c8](400a5c8)) * Role v1 readiness ([#2916](#2916)) ([32c7690](32c7690)) * Schema SDK upgrade ([#2945](#2945)) ([bca0836](bca0836)) * Streamlit v1 readiness ([#2930](#2930)) ([aa42260](aa42260)) ### 🔧 **Misc** * Remove deprecation from unsafe execute ([#2941](#2941)) ([ed712d7](ed712d7)) * Update documentation ([#2931](#2931)) ([da98bc3](da98bc3)) ### 🐛 **Bug fixes:** * ATTRIBUTE set(string) parsing for cortex search service ([#2953](#2953)) ([70a1c9a](70a1c9a)) * external function header parsing and add missing privileges ([#2961](#2961)) ([9d882fe](9d882fe)) * Fix sync_password field for Azure scim clients ([#2950](#2950)) ([6781133](6781133)) * Fix tests and relax warehouse validations ([#2959](#2959)) ([dd01ce9](dd01ce9)), closes [#2948](#2948) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>
Add missing session parameters (before redesigning user resource)