add unit tests to handle mutual exclusion #22352

Zlaticanin · 2023-08-15T19:55:46Z

No description provided.

raymonstah · 2023-08-15T20:10:05Z

builtin/logical/database/rotation_test.go

+		rotationPeriod   interface{}
+		rotationWindow   interface{}


can we use string here or was there a reason these types needed to be interface{}?

It look like these are interface{} so that we can set nil in the request data if they are unset in a given test case. Alternatively, we could make each test case define it's own data map?

Similar to:

vault/builtin/logical/database/path_roles_test.go

Lines 259 to 262 in d6b7e5b

account: map[string]interface{}{

"username": dbUser,

"rotation_period": "5400s",

},

That's correct, I did it so we can set it to nil. If we prefer/like the approach of setting data maps for each test case better, I'd be happy to update it

I think using a map might be a little more common for table tests and I probably prefer that style. But either way is ok

Sounds good, I'll update it 👍

github-actions · 2023-08-15T20:10:43Z

Build Results:
All builds succeeded! ✅

github-actions · 2023-08-15T20:21:47Z

CI Results:
All Go tests succeeded! ✅

fairclothjm · 2023-08-15T20:46:47Z

builtin/logical/database/rotation_test.go

+		wantErr          bool
+	}{
+		{
+			name:             "test-role-1",


nit: can we make the names more descriptive of what they are testing? Similar to

vault/builtin/logical/database/path_roles_test.go

Lines 43 to 48 in d6b7e5b

name: "role with invalid credential type",

args: args{

credentialType: v5.CredentialType(10),

},

wantErr: true,

},

fairclothjm

LGTM! Just a few suggestions but nothing blocking

* add unit tests to handle mutual exclusion * revert rotation_test.go and add missing test case to path_roles_test.go

* add rotation_schedule field to db backend * add cron schedule field * use priority queue with scheduled rotation types * allow marshalling of cron schedule type * return warning on use of mutually exclusive fields * handle mutual exclusion of rotation fields (#22306) * handle mutual exclusion of rotation fields * fix import * adv ttl mgmt: add rotation_window field (#22303) * adv ttl mgmt: add rotation_window field * do some rotation_window validation and add unit tests * adv ttl mgmt: Ensure initialization sets appropriate rotation schedule (#22341) * general cleanup and refactor rotation type checks * make NextRotationTime account for the rotation type * add comments * add unit tests to handle mutual exclusion (#22352) * add unit tests to handle mutual exclusion * revert rotation_test.go and add missing test case to path_roles_test.go * adv ttl mgmt: add tests for init queue (#22376) * Vault 18908/handle manual rotation (#22389) * support manual rotation for schedule based roles * update description and naming * adv ttl mgmt: consider rotation window (#22448) * consider rotation window ensure rotations only occur within a rotation window for schedule-based rotations * use helper method to set priority in rotateCredential * fix bug with priority check * remove test for now * add and remove comments * add unit tests for manual rotation (#22453) * adv ttl mgmt: add tests for rotation_window * adv ttl mgmt: refactor window tests (#22472) * Handle GET static-creds endpoint (#22476) * update read static-creds endpoint to include correct resp data * return rotation_window if set * update * add changelog * add unit test for static-creds read endpoint (#22505) --------- Co-authored-by: Milena Zlaticanin <60530402+Zlaticanin@users.noreply.github.com>

…22531) * add rotation_schedule field to db backend * add cron schedule field * use priority queue with scheduled rotation types * allow marshalling of cron schedule type * return warning on use of mutually exclusive fields * handle mutual exclusion of rotation fields (#22306) * handle mutual exclusion of rotation fields * fix import * adv ttl mgmt: add rotation_window field (#22303) * adv ttl mgmt: add rotation_window field * do some rotation_window validation and add unit tests * adv ttl mgmt: Ensure initialization sets appropriate rotation schedule (#22341) * general cleanup and refactor rotation type checks * make NextRotationTime account for the rotation type * add comments * add unit tests to handle mutual exclusion (#22352) * add unit tests to handle mutual exclusion * revert rotation_test.go and add missing test case to path_roles_test.go * adv ttl mgmt: add tests for init queue (#22376) * Vault 18908/handle manual rotation (#22389) * support manual rotation for schedule based roles * update description and naming * adv ttl mgmt: consider rotation window (#22448) * consider rotation window ensure rotations only occur within a rotation window for schedule-based rotations * use helper method to set priority in rotateCredential * fix bug with priority check * remove test for now * add and remove comments * add unit tests for manual rotation (#22453) * adv ttl mgmt: add tests for rotation_window * adv ttl mgmt: refactor window tests (#22472) * Handle GET static-creds endpoint (#22476) * update read static-creds endpoint to include correct resp data * return rotation_window if set * update * add changelog * add unit test for static-creds read endpoint (#22505) * Add the ability to set seconds in cron schedule for testing purposes * update test so we don't use global var * update with suggestions --------- Co-authored-by: JM Faircloth <jmfaircloth@hashicorp.com> Co-authored-by: John-Michael Faircloth <fairclothjm@users.noreply.github.com>

add unit tests to handle mutual exclusion

11229ca

Zlaticanin requested a review from a team as a code owner August 15, 2023 19:55

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Aug 15, 2023

raymonstah approved these changes Aug 15, 2023

View reviewed changes

fairclothjm reviewed Aug 15, 2023

View reviewed changes

fairclothjm approved these changes Aug 15, 2023

View reviewed changes

revert rotation_test.go and add missing test case to path_roles_test.go

68112be

Zlaticanin requested a review from fairclothjm August 16, 2023 19:40

fairclothjm approved these changes Aug 16, 2023

View reviewed changes

Zlaticanin merged commit 7bcb038 into VAULT-17232/adv-ttl-mgmt Aug 16, 2023
91 checks passed

Zlaticanin deleted the VAULT-19035/add-unit-tests branch August 16, 2023 20:01

Zlaticanin added a commit that referenced this pull request Aug 22, 2023

add unit tests to handle mutual exclusion (#22352)

7a542fc

* add unit tests to handle mutual exclusion * revert rotation_test.go and add missing test case to path_roles_test.go

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add unit tests to handle mutual exclusion #22352

add unit tests to handle mutual exclusion #22352

Zlaticanin commented Aug 15, 2023

raymonstah Aug 15, 2023

fairclothjm Aug 15, 2023

Zlaticanin Aug 15, 2023 •

edited

Loading

fairclothjm Aug 15, 2023

Zlaticanin Aug 15, 2023

github-actions bot commented Aug 15, 2023

github-actions bot commented Aug 15, 2023

fairclothjm Aug 15, 2023

fairclothjm left a comment

	account: map[string]interface{}{
	"username": dbUser,
	"rotation_period": "5400s",
	},

	name: "role with invalid credential type",
	args: args{
	credentialType: v5.CredentialType(10),
	},
	wantErr: true,
	},

add unit tests to handle mutual exclusion #22352

add unit tests to handle mutual exclusion #22352

Conversation

Zlaticanin commented Aug 15, 2023

raymonstah Aug 15, 2023

Choose a reason for hiding this comment

fairclothjm Aug 15, 2023

Choose a reason for hiding this comment

Zlaticanin Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

fairclothjm Aug 15, 2023

Choose a reason for hiding this comment

Zlaticanin Aug 15, 2023

Choose a reason for hiding this comment

github-actions bot commented Aug 15, 2023

github-actions bot commented Aug 15, 2023

fairclothjm Aug 15, 2023

Choose a reason for hiding this comment

fairclothjm left a comment

Choose a reason for hiding this comment

Zlaticanin Aug 15, 2023 •

edited

Loading