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

add max_entry_size to sanitized config output #20044

Merged
merged 5 commits into from
Apr 14, 2023

Conversation

ccapurso
Copy link
Contributor

@ccapurso ccapurso commented Apr 7, 2023

This PR adds a raft sub-field to the sanitized config storage and HA storage details in order to expose Raft's max_entry_size.

@ccapurso ccapurso added this to the 1.13.2 milestone Apr 7, 2023
@ccapurso ccapurso marked this pull request as ready for review April 7, 2023 15:08
@ccapurso ccapurso requested a review from a team April 12, 2023 17:32
Copy link
Contributor

@hghaf099 hghaf099 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

defer ln.Close()
TestServerAuth(t, addr, token)
for _, tc := range cases {
tc := tc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should this be removed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is a common pattern to capture the loop variable inside a closure (see: #16872 for an example).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for calling that out. I meant to add t.Parallel to these which is why the capturing is present. I'll add parallelism.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See cc23ded

Copy link
Contributor

@mpalmi mpalmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Just one comment about another test-case. Feel free to ignore.

http/sys_config_state_test.go Outdated Show resolved Hide resolved
defer ln.Close()
TestServerAuth(t, addr, token)
for _, tc := range cases {
tc := tc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is a common pattern to capture the loop variable inside a closure (see: #16872 for an example).

expectedHAStorageOutput map[string]interface{}
}{
{
"raft storage",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: these test cases would be a bit more readable if we used named struct fields, e.g.

{
  name: "raft storage",
  storageConfig: &server.Storage{...},
  ...
}

Copy link
Contributor Author

@ccapurso ccapurso Apr 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll change that. GoLand has a UI treatment to identify which fields are which but that doesn't help those using a different IDE, or reading it in GitHub. Thanks for the feedback!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See ca39491

@ccapurso ccapurso requested a review from miagilepner April 13, 2023 13:44
@ccapurso ccapurso merged commit ca70274 into main Apr 14, 2023
@ccapurso ccapurso deleted the vault-15261-sanitized-config-max-entry-size branch April 14, 2023 13:52
ccapurso added a commit that referenced this pull request Apr 17, 2023
* add max_entry_size to sanitized config output

* add changelog entry

* add test parallelism

* add inmem test case

* use named struct fields for TestSysConfigState_Sanitized cases
ccapurso added a commit that referenced this pull request Apr 17, 2023
* add max_entry_size to sanitized config output

* add changelog entry

* add test parallelism

* add inmem test case

* use named struct fields for TestSysConfigState_Sanitized cases
ccapurso added a commit that referenced this pull request Apr 17, 2023
* add max_entry_size to sanitized config output

* add changelog entry

* add test parallelism

* add inmem test case

* use named struct fields for TestSysConfigState_Sanitized cases
ccapurso added a commit that referenced this pull request Apr 17, 2023
* add max_entry_size to sanitized config output

* add changelog entry

* add test parallelism

* add inmem test case

* use named struct fields for TestSysConfigState_Sanitized cases
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.

4 participants