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 HierarchyChangeAuth command #357

Merged
merged 1 commit into from
May 14, 2024
Merged

Conversation

novag
Copy link
Contributor

@novag novag commented Apr 1, 2024

see definition in Part 3, Commands, section 24.8

@novag novag requested review from alexmwu, jkl73 and a team as code owners April 1, 2024 11:06
Copy link

google-cla bot commented Apr 1, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Member

@chrisfenner chrisfenner left a comment

Choose a reason for hiding this comment

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

Thanks for the change! I had a couple of minor comments, otherwise looks ready to go.

tpm2/test/hierarchy_change_auth_test.go Outdated Show resolved Hide resolved
tpm2/test/hierarchy_change_auth_test.go Outdated Show resolved Hide resolved
tpm2/test/hierarchy_change_auth_test.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@novag novag left a comment

Choose a reason for hiding this comment

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

Thanks! In which cases would you want the t.Fatal to be a t.Error?

tpm2/test/hierarchy_change_auth_test.go Outdated Show resolved Hide resolved
tpm2/test/hierarchy_change_auth_test.go Outdated Show resolved Hide resolved
@chrisfenner
Copy link
Member

To be more precise, I think lines 31, 45, and 62 in change_auth_test.go could all be t.Errorf. There aren't currently more test assertions after these lines so it's not a big deal. Either Fatalf or Errorf will fail the test, but Errorf is preferred unless the failure means the test should abort (https://google.github.io/styleguide/go/decisions#keep-going).

see definition in Part 3, Commands, section 24.8
@chrisfenner
Copy link
Member

Thank you @novag for the change and for the updates in response to my minor feedback items!

@chrisfenner chrisfenner merged commit 58e3e47 into google:main May 14, 2024
4 checks passed
mikaolss pushed a commit to mikaolss/go-tpm that referenced this pull request Jul 11, 2024
see definition in Part 3, Commands, section 24.8
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.

2 participants