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.

Sign up for GitHub

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

Jump to bottom

chore: update err msg for policy verification #294

Merged
merged 2 commits into from
Apr 7, 2023
Merged

chore: update err msg for policy verification #294

merged 2 commits into from
Apr 7, 2023

Conversation

qweeah
Copy link
Contributor

@qweeah qweeah commented Mar 28, 2023

This PR rephrased error message returned during trust policy verification.

patrickzheng200
patrickzheng200 reviewed Mar 28, 2023
View reviewed changes
Copy link
Contributor

@patrickzheng200 patrickzheng200 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@qweeah Have you checked the following?

  1. run go test -v . under trustpolicy dir. All tests need to be passed.
  2. run the notation CLI e2e test again, because there are test cases expecting this error message. Changing the error message would fail the e2e test. All e2e tests need to be passed as well.

@qweeah
Copy link
Contributor Author

qweeah commented Mar 28, 2023 •
edited
Loading

Will update UT.

2. Changing the error message would fail the e2e test. All e2e tests need to be passed as well.

Shouldn't that be done when bumping notation-go? If I update e2e spec, then it will fail without the right version of notation-go.

@patrickzheng200
Copy link
Contributor

  1. Changing the error message would fail the e2e test. All e2e tests need to be passed as well.

Shouldn't that be done when bumping notation-go?

oh yeah, if you need to change the e2e, you could include the change in this PR: notaryproject/notation#593 because this change is connected closely with your CLI trust policy command.
Merge the notation-go PR first -> go mod tidy on notation CLI side -> merge the notation CLI PR.

patrickzheng200
patrickzheng200 previously approved these changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@patrickzheng200 patrickzheng200 left a comment

Choose a reason for hiding this comment

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

LGTM

patrickzheng200
patrickzheng200 approved these changes Apr 6, 2023
View reviewed changes
Copy link
Contributor

@patrickzheng200 patrickzheng200 left a comment

Choose a reason for hiding this comment

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

LGTM

priteshbandi
priteshbandi approved these changes Apr 6, 2023
View reviewed changes
Copy link
Contributor

@priteshbandi priteshbandi left a comment

Choose a reason for hiding this comment

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

LGTM

shizhMSFT
shizhMSFT approved these changes Apr 7, 2023
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@shizhMSFT
Copy link
Contributor

I'm not able to merge this PR due to
image
caused by recent change by notaryproject/notation-core-go#135

@shizhMSFT shizhMSFT closed this Apr 7, 2023
@shizhMSFT shizhMSFT reopened this Apr 7, 2023
@shizhMSFT
Copy link
Contributor

Updated the branch policy to match the workflow defined in notation-core-go.

@shizhMSFT shizhMSFT merged commit 753b6b1 into notaryproject:main Apr 7, 2023
@qweeah
Copy link
Contributor Author

qweeah commented Apr 7, 2023

@qweeah Have you checked the following?

  1. run go test -v . under trustpolicy dir. All tests need to be passed.
  2. run the notation CLI e2e test again, because there are test cases expecting this error message. Changing the error message would fail the e2e test. All e2e tests need to be passed as well.

@patrickzheng200 I bumped notation-go including this change and ran an E2E in my own fork, looks like no test will fail thus no further change required on CLI side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@priteshbandi priteshbandi priteshbandi approved these changes

@byronchien byronchien byronchien approved these changes

@shizhMSFT shizhMSFT shizhMSFT approved these changes

@patrickzheng200 patrickzheng200 patrickzheng200 approved these changes

@gokarnm gokarnm Awaiting requested review from gokarnm gokarnm is a code owner

@JeyJeyGao JeyJeyGao Awaiting requested review from JeyJeyGao JeyJeyGao is a code owner

@justincormack justincormack Awaiting requested review from justincormack

@NiazFK NiazFK Awaiting requested review from NiazFK NiazFK is a code owner

@rgnote rgnote Awaiting requested review from rgnote rgnote is a code owner

@SteveLasker SteveLasker Awaiting requested review from SteveLasker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@qweeah @patrickzheng200 @shizhMSFT @priteshbandi @byronchien