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

refactor: Implementing sigs.k8s.io YAML to remove .proto yaml annotations #9780

Merged
merged 42 commits into from
Sep 24, 2021

Conversation

lukerhoads
Copy link
Contributor

@lukerhoads lukerhoads commented Jul 27, 2021

Description

Draft of: #9705

Started off with changing codec MarshalYaml function to directly go from JSON to yaml using the new library. Replaced the only usage of UnmarshalYaml per request.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@lukerhoads lukerhoads changed the title [feat] Implementing sigs.k8s.io YAML to remove .proto yaml annotations feat: Implementing sigs.k8s.io YAML to remove .proto yaml annotations Jul 30, 2021
@lukerhoads
Copy link
Contributor Author

lukerhoads commented Jul 30, 2021

If some YAML marshallers are being replaced, then I guess all of them would have to be for consistency. There are still some places where gopkg.in/yaml.v2 would be necessary, such as for the Marshaler interface implementation. Other than that, straight up replacing would probably be fine. Take this with a grain of salt though, and please correct me as I am most likely incorrect 😂

@ruhatch ruhatch changed the title feat: Implementing sigs.k8s.io YAML to remove .proto yaml annotations refactor: Implementing sigs.k8s.io YAML to remove .proto yaml annotations Jul 30, 2021
Copy link
Contributor

@ruhatch ruhatch 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 so far, thanks for the work!

I think we can replace the remaining gopkg.in instances:

  • client/context.go we should be able to use JSONToYAML method from the new library, but we'll need to manually test that to confirm
  • I think we can remove the yaml.Marshaler references
  • There are about 7 other files where it shows up and I think we can replace it in all of those

@github-actions github-actions bot added C:x/evidence C:x/genutil genutil module issues labels Jul 30, 2021
@lgtm-com
Copy link

lgtm-com bot commented Jul 30, 2021

This pull request introduces 1 alert when merging 985390d into 36ab23a - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@lukerhoads
Copy link
Contributor Author

lukerhoads commented Sep 3, 2021

The nested ConsensusPubkey is causing errors when being marshalled. When the any is being constructed in NewValidator, it is able to be marshalled. But when it is marshalled in the validator string function, it fails with the error "this is likely because amino is being used directly (instead of codec.LegacyAmino which is preferred) or UnpackInterfacesMessage is not defined for some type which contains a protobuf Any either directly or via one of its members. To see a stacktrace of where the error is coming from, set the var Debug = true in codec/types/compat.go" - I feel I am close but I am clueless with protobuf

edit: idk what this means or how to do it, could I please get help

@ruhatch
Copy link
Contributor

ruhatch commented Sep 4, 2021

The nested ConsensusPubkey is causing errors when being marshalled. When the any is being constructed in NewValidator, it is able to be marshalled. But when it is marshalled in the validator string function, it fails with the error "this is likely because amino is being used directly (instead of codec.LegacyAmino which is preferred) or UnpackInterfacesMessage is not defined for some type which contains a protobuf Any either directly or via one of its members. To see a stacktrace of where the error is coming from, set the var Debug = true in codec/types/compat.go" - I feel I am close but I am clueless with protobuf

edit: idk what this means or how to do it, could I please get help

@AmauryM would you be able to help with this?

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

thanks @lukerhoads for this PR, and sorry for the delay!

Just had a look, and proposed a fix for the failing test. Could you also merge/rebase master into this branch?

proto/cosmos/vesting/v1beta1/tx.proto Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
x/bank/types/params_test.go Outdated Show resolved Hide resolved
x/bank/types/params_test.go Outdated Show resolved Hide resolved
x/staking/types/validator.go Show resolved Hide resolved
@lukerhoads
Copy link
Contributor Author

This last test seems to be failing because the sigs.k8s.io yaml package does not call the MarshalYAML function of the StdSignature. The gopkg yaml does, though, as it uses its own type MarshalYAML. This means that we should just call the MarshalYAML function directly to avoid using gopkg yaml.

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

this overall lgtm! @lukerhoads could you fix the conflict and the failing test? We can merge it right after!

x/bank/types/params_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@lukerhoads
Copy link
Contributor Author

At school right now - when I get home I will get on WSL so I can properly fix the last test, or you guys could take over (as I have been rlly slow and don't have a very good idea of how to fix the TestGRPCWebTestSuite error

@lukerhoads
Copy link
Contributor Author

I have no idea why all of the builds are failing, I am not at all experienced with this kind of error

@amaury1093
Copy link
Contributor

I have no idea why all of the builds are failing, I am not at all experienced with this kind of error

I believe you simply need to run go mod tidy and commit the changes.

@codecov
Copy link

codecov bot commented Sep 24, 2021

Codecov Report

Merging #9780 (a20180d) into master (f265287) will decrease coverage by 0.01%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9780      +/-   ##
==========================================
- Coverage   63.67%   63.65%   -0.02%     
==========================================
  Files         573      573              
  Lines       53764    53762       -2     
==========================================
- Hits        34235    34223      -12     
- Misses      17582    17590       +8     
- Partials     1947     1949       +2     
Impacted Files Coverage Δ
client/keys/parse.go 77.01% <ø> (ø)
client/keys/utils.go 41.93% <ø> (ø)
types/address.go 66.86% <ø> (-1.83%) ⬇️
types/result.go 80.40% <ø> (ø)
version/command.go 83.87% <ø> (ø)
x/auth/types/account.go 75.42% <ø> (ø)
x/auth/types/params.go 78.49% <ø> (ø)
x/auth/vesting/types/period.go 0.00% <ø> (ø)
x/auth/vesting/types/vesting_account.go 87.50% <ø> (ø)
x/capability/types/types.go 100.00% <ø> (ø)
... and 22 more

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Sep 24, 2021
@mergify mergify bot merged commit bf11b1b into cosmos:master Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants