pokt-network / poktroll

The official Shannon upgrade implementation of the Pocket Network Protocol implemented using Rollkit.dev
MIT License
15 stars 6 forks source link

[SDK] Track pb.go files #544

Closed red-0ne closed 1 month ago

red-0ne commented 1 month ago

Summary

This PR adds tracking to .pb.go generated files and removes the corresponding entry from .gitignore

Issue

In order to use the generated types corresponding to proktroll protobuf definitions from external repositories, the generated .pb.go files need to be added to the repo.

This is currently the only viable solution to decouple the SDK and extract it to its own repo.

Type of change

Select one or more:

Testing

Documentation changes (only if making doc changes)

Local Testing (only if making code changes)

PR Testing (only if making code changes)

Sanity Checklist

okdas commented 1 month ago

@Olshansk I agree. If we remove proto_regen from CI, we will achieve reproducible builds. Let's remove this step.

We can introduce another CI step that will fail if there's a difference between the committed pb files and the protobufs generated by CI. This way, we can catch situations where .proto files were changed but .pb.go files were not updated (or were updated incorrectly).

Does that make sense? If so, I'll create a ticket to work on that.

Olshansk commented 1 month ago

We can introduce another CI step that will fail if there's a difference between the committed pb files and the protobufs generated by CI

@red-0ne let's add a TODO_IMPROVE for ☝️ but seems overkill for now

github-actions[bot] commented 1 month ago

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. If you just created a pull request, you might need to push another commit to produce a container image DevNet can utilize to spin up infrastructure. You can use make trigger_ci to push an empty commit.

red-0ne commented 1 month ago

Reviewdog checks are failing due to big diff but unit and e2e tests are passing.

image

Merging