-
Notifications
You must be signed in to change notification settings - Fork 420
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: SCIM integration v1 readiness #2846
Conversation
Integration tests failure for 0709ec2afdf9af42beaa836026e03446700c5aef |
Integration tests failure for dbe65c93ed7c1b57f7f539018b68f22cff4a7262 |
Integration tests failure for dd341afc97af3df0320d3ee327c86aec07df02e3 |
Integration tests success for 89343b5d25f0c711e4d42e0d308ad6d0befbf05c |
} | ||
|
||
// AssertErrorContainsPartsFunc returns a function asserting error message contains each string in parts | ||
func AssertErrorContainsPartsFunc(t *testing.T, parts []string) resource.ErrorCheckFunc { |
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.
so why do we need a second method doing almost the same thing as the one above?
And also: this one should also be tested, but because assertions are a bit trickier to test (but we have examples in our repository), it does not have to be a part of this PR.
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.
Because for some errors, e.g. missing required fields in config, the order of errors is not deterministic. We get error message as a simple error
so the alternative is to parse it and sort messages and I'm not a fan of such implementation. Also, function signatures are different because they are used as resource.TestStep.ExpectError
and resource.TestCase.ErrorCheck
respectively.
I can add tests in the next PR.
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.
Okay , but why:
- do we need
ErrorCheck
versusExpectError
? Why can't we just use expect error too here? - do we care about the ordering of errors at all then? Maybe one assertion not caring about the ordering would be enough?
pkg/resources/scim_integration.go
Outdated
ValidateFunc: validation.StringInSlice(sdk.AsStringList([]sdk.ScimSecurityIntegrationRunAsRoleOption{ | ||
sdk.ScimSecurityIntegrationRunAsRoleOktaProvisioner, sdk.ScimSecurityIntegrationRunAsRoleAadProvisioner, sdk.ScimSecurityIntegrationRunAsRoleGenericScimProvisioner, | ||
}), true), | ||
DiffSuppressFunc: ignoreTrimSpaceSuppressFunc, |
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 not clear with the previous comment:
- Yes, they must be capitalized and we handle it on the SDK level.
- We agreed that we allow users to put e.g. lower cased values (this was the previous behavior).
- With the old implementation (before your changes) it was allowed, and diff suppression handled also lowercase correctly (and my comment was only about that using
ignoreTrimSpaceSuppressFunc
breaks it)
Integration tests failure for 88a2375010a421e2de2433567d3b9acaf4823f59 |
Integration tests failure for 88a2375010a421e2de2433567d3b9acaf4823f59 |
Integration tests failure for a8e0765a7e8b138a421b7c0cf07d5a96c7707f5e |
Integration tests failure for 00f0b31d20e29fc400956f7ba374f8a0880e0cbb |
00f0b31
to
7533f4e
Compare
Integration tests failure for 7533f4e54c7512641cb0a64957eb5bd070448246 |
3403ca1
to
c2aa70e
Compare
c2aa70e
to
e54ed9a
Compare
Integration tests failure for 3403ca1f831cda790c6ef72163088ebcf0aaf67e |
Integration tests failure for c2aa70ece0a46f6382ab06fa51d35854941fdadf |
Integration tests failure for e54ed9a97f68800752fe72e249c3c7c779c9f27c |
Integration tests failure for 7d9b62c4a95519b8369a0e74b5d31f52701d967e |
Integration tests success for 7d9b62c4a95519b8369a0e74b5d31f52701d967e |
@@ -232,21 +221,21 @@ func TestAcc_ScimIntegration_migrateFromVersion091(t *testing.T) { | |||
Source: "Snowflake-Labs/snowflake", | |||
}, | |||
}, | |||
Config: scimIntegrationv091(id.Name(), role.Name), | |||
Config: scimIntegrationv091(id.Name(), role.Name()), |
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.
names changed (we already have v0.92)
@@ -213,10 +204,8 @@ func TestAcc_ScimIntegration_InvalidIncomplete(t *testing.T) { | |||
} | |||
|
|||
func TestAcc_ScimIntegration_migrateFromVersion091(t *testing.T) { |
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.
let's add one more test for migration (with setting enabled to false after and expecting changes) - it may be added in the next PR
Merging, unresolved comments will be addressed/discussed in the next PR. |
🤖 I have created a release *beep* *boop* --- ## [0.93.0](v0.92.0...v0.93.0) (2024-07-10) ### 🎉 **What's new:** * Add OAUTH integration for custom clients ([#2908](#2908)) ([d9b557f](d9b557f)) * Add oauth integration for partner applications ([#2912](#2912)) ([91788e5](91788e5)) * Add support for cortex search service ([#2860](#2860)) ([43aa89f](43aa89f)) * API Authentication integration v1 readiness ([#2898](#2898)) ([91931da](91931da)) * External Oauth integration v1 readiness ([#2907](#2907)) ([ed237c3](ed237c3)) * Generate show outputs with mappers ([#2886](#2886)) ([1cada88](1cada88)) * Introduce security integrations datasource ([#2892](#2892)) ([7f6c657](7f6c657)) * SAML2 integration v1 readiness ([#2868](#2868)) ([d0c136d](d0c136d)) * SCIM integration v1 readiness ([#2846](#2846)) ([269df6b](269df6b)) * Security integrations datasource v1 readiness ([#2913](#2913)) ([d10474a](d10474a)) * standard database v1 readiness ([#2842](#2842)) ([3c11953](3c11953)) * Warehouse redesign final touches ([#2900](#2900)) ([0eab636](0eab636)) * Warehouse redesign part1 ([#2864](#2864)) ([6664457](6664457)) * Warehouse redesign part2 ([#2887](#2887)) ([1aaf417](1aaf417)) * Warehouse redesign part3 ([#2890](#2890)) ([873a1ed](873a1ed)) * Warehouse redesign part4 ([#2893](#2893)) ([d525fd9](d525fd9)) ### 🔧 **Misc** * Add documentation on unset and defaults ([#2882](#2882)) ([85a7836](85a7836)) * apply minor database changes ([#2872](#2872)) ([6ccac59](6ccac59)) * Apply new resource conventions to scim integration ([#2891](#2891)) ([e11e608](e11e608)) * Improve generator template organization ([#2820](#2820)) ([5035e2f](5035e2f)) * Nuke stale objects ([#2869](#2869)) ([9c4a117](9c4a117)) * Show a possible solution for [#2877](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2877) ([#2878](#2878)) ([6fb437b](6fb437b)) * Validations cleanup and old grants removal ([#2884](#2884)) ([05b7eee](05b7eee)) ### 🐛 **Bug fixes:** * Add disclaimers and fix tests ([#2905](#2905)) ([1deaedc](1deaedc)) * Fix cortex search service ([#2904](#2904)) ([763d06c](763d06c)) * use suppressQuoting to fix stage file_format permadiff ([#2885](#2885)) ([fd70f6e](fd70f6e)) --- 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>
Test Plan
References
https://docs.snowflake.com/en/sql-reference/sql/create-security-integration-scim