Skip to content

Comments

fix: update plugin to add ContractVersion#207

Merged
shizhMSFT merged 7 commits intonotaryproject:mainfrom
JeyJeyGao:fix/plugin_add_contract_version
Nov 23, 2022
Merged

fix: update plugin to add ContractVersion#207
shizhMSFT merged 7 commits intonotaryproject:mainfrom
JeyJeyGao:fix/plugin_add_contract_version

Conversation

@JeyJeyGao
Copy link
Contributor

@JeyJeyGao JeyJeyGao commented Nov 17, 2022

  • Add ContractVersion argument when executing plugin request
  • Check ContractVersion when getting plugin metadata
  • Moved the path existence check from manager.Get() to NewCLIPlugin()

Test:
Added invalid contract version unit test.

Signed-off-by: Junjie Gao junjiegao@microsoft.com

@JeyJeyGao JeyJeyGao requested review from patrickzheng200, priteshbandi, rgnote and shizhMSFT and removed request for shizhMSFT November 17, 2022 02:35
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2022

Codecov Report

Merging #207 (03ff11a) into main (38198df) will increase coverage by 0.74%.
The diff coverage is 93.15%.

@@            Coverage Diff             @@
##             main     #207      +/-   ##
==========================================
+ Coverage   65.90%   66.64%   +0.74%     
==========================================
  Files          23       23              
  Lines        1575     1559      -16     
==========================================
+ Hits         1038     1039       +1     
+ Misses        469      455      -14     
+ Partials       68       65       -3     
Impacted Files Coverage Δ
plugin/manager.go 77.77% <ø> (-6.44%) ⬇️
plugin/proto/metadata.go 81.81% <ø> (-12.78%) ⬇️
verifier/helpers.go 67.56% <ø> (+4.64%) ⬆️
signer/plugin.go 23.31% <28.57%> (+0.63%) ⬆️
plugin/plugin.go 94.82% <100.00%> (+3.39%) ⬆️
verifier/verifier.go 75.06% <100.00%> (-0.07%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

Should we also check the contract version in the response by the package plugin?

@JeyJeyGao JeyJeyGao force-pushed the fix/plugin_add_contract_version branch from f0181c4 to fc4834d Compare November 17, 2022 03:31
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

@JeyJeyGao JeyJeyGao force-pushed the fix/plugin_add_contract_version branch 2 times, most recently from 335e26a to ffaa0fe Compare November 17, 2022 05:06
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
Copy link
Contributor

@JeyJeyGao Please rebase the PR.

@JeyJeyGao JeyJeyGao force-pushed the fix/plugin_add_contract_version branch from b90ee82 to 55aa3e5 Compare November 18, 2022 03:06
@JeyJeyGao JeyJeyGao force-pushed the fix/plugin_add_contract_version branch 4 times, most recently from 79e293b to da4ce4b Compare November 18, 2022 12:39
@yizha1 yizha1 added this to the RC-1 milestone Nov 18, 2022
@JeyJeyGao JeyJeyGao force-pushed the fix/plugin_add_contract_version branch from c8d8413 to d8e195b Compare November 21, 2022 01:18
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

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
@JeyJeyGao JeyJeyGao force-pushed the fix/plugin_add_contract_version branch from 59deb50 to 03ff11a Compare November 23, 2022 06:40
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 shizhMSFT merged commit 7e391f7 into notaryproject:main Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants