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

ci: introduce proto breakage detector (#2671) #2684

Merged
merged 2 commits into from
Oct 23, 2023
Merged

Conversation

cmwaters
Copy link
Contributor

This ports over #2671 to `main

rootulp
rootulp previously approved these changes Oct 16, 2023
Makefile Outdated
## proto-check-breaking: Check if there are any breaking change to protobuf definitions.
proto-check-breaking:
@echo "--> Checking if Protobuf definitions have any breaking changes"
@$(DOCKER_BUF) breaking --against $(HTTPS_GIT)#branch=v1.x
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't it make more sense to test against the target branch instead of always main?

Also, why https://github.com/celestiaorg/celestia-app.git references #branch=v1.x as specified in the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point. I just directly ported over the commit although theoretically this is also accurate as we should never break from v1 onwards

Copy link
Collaborator

Choose a reason for hiding this comment

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

[discussion] I had the same question as Rachid but assumed that main intentionally wants to preserve Protobuf compatability with v1.x

Do we want to check for Protobuf breaking changes against main and v1.x? If we can't do this with one command maybe:

	@$(DOCKER_BUF) breaking --against $(HTTPS_GIT)#branch=v1.x
	@$(DOCKER_BUF) breaking --against $(HTTPS_GIT)#branch=main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So long as we haven't already broken protos there shouldn't be any difference between checking it against v1.x and main so we needn't do both.

@codecov-commenter
Copy link

Codecov Report

Merging #2684 (ff47ee9) into main (40385b9) will decrease coverage by 1.22%.
Report is 3 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2684      +/-   ##
==========================================
- Coverage   21.11%   19.89%   -1.22%     
==========================================
  Files         138      139       +1     
  Lines       15767    16695     +928     
==========================================
- Hits         3329     3322       -7     
- Misses      12115    13051     +936     
+ Partials      323      322       -1     

see 77 files with indirect coverage changes

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

nice, thanks for thinking of this

@cmwaters cmwaters merged commit 35074ec into main Oct 23, 2023
32 of 33 checks passed
@cmwaters cmwaters deleted the cal/proto-breakage branch October 23, 2023 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants