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

Making CI for testing generated proto correctness #1892

Merged
merged 41 commits into from
Jul 4, 2022
Merged

Conversation

xBalbinus
Copy link
Contributor

@xBalbinus xBalbinus commented Jun 28, 2022

Closes: #1256

What is the purpose of the change

This CI will help us catch whenever make proto-all wasn't ran via a job that does make proto-all,and returns an error if there is a diff betrween the post make proto-all output, and any .proto or .pb.go file, as well as tool-chain related errors.

Brief Changelog

Added job in check-generated.yml that fails if make proto-all wasn't done before PR

Testing and Verifying

(Please pick one of the following options)

This change added tests and can be verified as follows:

(example:)

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@github-actions github-actions bot added the T:CI label Jun 28, 2022
@xBalbinus xBalbinus marked this pull request as ready for review June 29, 2022 14:16
@xBalbinus xBalbinus requested a review from a team June 29, 2022 14:16
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

Can we make it so that we do not check protobuf generated code for go.mod and go.sum?


- name: "Check protobuf generated code"
run: |
set -euo pipefail
Copy link
Contributor

Choose a reason for hiding this comment

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

We should just put this all in a script under contrib/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sgtm! doing so.

@xBalbinus
Copy link
Contributor Author

Looking into the go.mod issue!

Comment on lines 31 to 32
with:
password: ${{ secrets.ADD_TO_PROJECT_PAT }}
Copy link
Member

@ValarDragon ValarDragon Jun 30, 2022

Choose a reason for hiding this comment

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

Why is this needed?

This also should be a different access token, lets set that up differently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This most likely isn't - sorry I'm just testing out some stuff to implement the feedback above

Copy link
Member

Choose a reason for hiding this comment

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

ah gotcha

@ValarDragon
Copy link
Member

Not sure what you mean by go.mod and go.sum @mattverse ?

We should have in another PR, make a second job that checks go mod & go sum correctness

@mattverse
Copy link
Member

Oh I was saying that this "shouldn't" include go.mod and go.sum, since we're maintaining them separately?

@mattverse
Copy link
Member

Seems like the CI is breaking for this PR? Is that an expected behaviour resulting from this PR not being merged yet?

@xBalbinus
Copy link
Contributor Author

xBalbinus commented Jul 1, 2022 via email

@xBalbinus
Copy link
Contributor Author

Passing all tests, ready for merge

Comment on lines 10 to 11
branches:
- main
Copy link
Member

Choose a reason for hiding this comment

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

Can we do *, otherwise LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and dusted 👍

Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

let's go 🔥

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

utACK 🎉

@alexanderbez alexanderbez merged commit 0956d89 into main Jul 4, 2022
@alexanderbez alexanderbez deleted the xiangan/ci branch July 4, 2022 13:41
@ValarDragon ValarDragon added the A:backport/v10.x backport patches to v10.x branch label Jul 15, 2022
mergify bot pushed a commit that referenced this pull request Jul 15, 2022
(cherry picked from commit 0956d89)

# Conflicts:
#	chain.schema.json
ValarDragon added a commit that referenced this pull request Jul 15, 2022
…2074)

* Making CI for testing generated proto correctness (#1892)

(cherry picked from commit 0956d89)

# Conflicts:
#	chain.schema.json

* Fix proto

Co-authored-by: Xiangan He <[email protected]>
Co-authored-by: Dev Ojha <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v10.x backport patches to v10.x branch T:CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make CI for testing generated proto correctness
4 participants