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

feat: Add missing session parameters #2936

Merged
merged 5 commits into from
Jul 16, 2024

Conversation

sfc-gh-asawicki
Copy link
Collaborator

Add missing session parameters (before redesigning user resource)

Copy link

Integration tests cancelled for f17f616fd3979e07e3165e097a4a6e315c9efa36

@sfc-gh-asawicki sfc-gh-asawicki marked this pull request as ready for review July 12, 2024 13:30
Copy link

Integration tests failure for 7e27aaef906fa176c592131aef14f3002cf7469f

sessionParameters.LockTimeout = Pointer(v)
err = setIntegerValue(parameter, value, &sessionParameters.LockTimeout)
case SessionParameterLogLevel:
sessionParameters.LogLevel = Pointer(LogLevel(value))
Copy link
Collaborator

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

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, ....}
}

pkg/sdk/parameters_impl.go Show resolved Hide resolved
pkg/sdk/parameters_impl.go Show resolved Hide resolved
Copy link

Integration tests failure for 5bd0bcd3d4919a906dbc8720817ee20fe04b0d82

Copy link
Collaborator

@sfc-gh-jmichalak sfc-gh-jmichalak left a 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 {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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 :)

@sfc-gh-asawicki sfc-gh-asawicki merged commit 4ce662d into main Jul 16, 2024
8 of 9 checks passed
@sfc-gh-asawicki sfc-gh-asawicki deleted the add-missing-session-parameters branch July 16, 2024 14:00
sfc-gh-asawicki added a commit that referenced this pull request Jul 22, 2024
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
sfc-gh-jcieslak pushed a commit that referenced this pull request Jul 26, 2024
🤖 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>
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.

3 participants