ci: ensure the generated code is up-to-date #14664
Conversation
spacewander
force-pushed
the
ra1
branch
3 times, most recently
from
7047058 to
a7f4c3d
Compare
Codecov Report
@@ Coverage Diff @@
## main #14664 +/- ##
==========================================
- Coverage 75.53% 75.51% -0.02%
==========================================
Files 457 457
Lines 37292 37292
==========================================
- Hits 28169 28162 -7
- Misses 7351 7360 +9
+ Partials 1772 1770 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
scripts/verify_genproto.sh
Outdated
| set -o pipefail | ||
|
|
||
| ./scripts/genproto.sh | ||
| diff=$(git diff --name-only | grep -c ".pb.") |
There was a problem hiding this comment.
This implementation checks if there is any uncommitted proto files, this should work for CI, but not for local development.
I think this should be ok, but maybe you can figure out if we could output ./scripts/genproto.sh to tmp directory and compare it's contents to local files.
| echo "Failed genproto-verification!" >&2 | ||
| echo "* Found changed files $(git diff --name-only | grep '.pb.')" >&2 | ||
| echo "* Please rerun genproto.sh after changing *.proto file" >&2 | ||
| echo "* Run ./scripts/genproto.sh" >&2 |
There was a problem hiding this comment.
This recommendation only makes sense when command is run in CI.
When running locally user needs to just commit the changes.
I think we should explain that user should run ./scripts/genproto.sh only if the commend was run in CI and then commit the changes.
|
Thanks for looking into this! |
See etcd-io#14612 (comment) Signed-off-by: spacewander <[email protected]>
| tmpWorkDir=$(mktemp -d -t 'twd.XXXXXX') | ||
| mkdir "$tmpWorkDir/etcd" | ||
| tmpWorkDir="$tmpWorkDir/etcd" | ||
| cp -r . "$tmpWorkDir" |
There was a problem hiding this comment.
Coping whole repo seems like a lot of work. Could we maybe just copy minimal set of files needed (*.proto)?
There was a problem hiding this comment.
I tried it, but the genproto.sh requires too many files (not just *.proto).
There was a problem hiding this comment.
Could we then simplify genproto.sh so it can be run on non local directory?
There was a problem hiding this comment.
It is not so easy, as the genproto.sh requires some Go files, extra shell scripts in the scripts directory, the Documentation directory, and so on.
| pushd "$tmpWorkDir" | ||
| git add -A && git commit -m init | ||
| ./scripts/genproto.sh | ||
| diff=$(git diff --numstat | awk '{print $3}') |
There was a problem hiding this comment.
This script will still compare generated proto versus proto committed, so there is no difference from older script.
There was a problem hiding this comment.
git diff doesn't include the committed change.
For changed but not committed changes, they are committed in the prev line git add -A && git commit -m init, so the diff happens in a fresh git repo.
There was a problem hiding this comment.
@serathius
Sorry for affecting your review. I just force-push a new commit that handles a case that git commit will fail.
There was a problem hiding this comment.
I'm little worried about git shenanigans (add commit), it makes the code very fragile.
Have you considered avoiding them? I will not push too much as overall change should be beneficial, just worried about over-complicating verify_genproto.sh if we could maybe simplify genproto.sh
There was a problem hiding this comment.
I have tried using regular diff. But the genproto.sh will generate some files which are ignored by .gitignore. And as regular diff doesn't support .gitignore, we have to maintain an additional ignore list...
There was a problem hiding this comment.
Ok, I think we are good enough. We can iterate on the code.
Signed-off-by: spacewander <[email protected]>
| @@ -15,6 +15,6 @@ fi | |||
| echo "Failed proto-annotations-verification!" >&2 | |||
| echo "If you are adding new proto fields/messages that will be included in raft log:" >&2 | |||
| echo "* Please add etcd_version annotation in *.proto file with next etcd version" >&2 | |||
| echo "* Run ./scripts/getproto.sh" >&2 | |||
| echo "* Run ./scripts/genproto.sh" >&2 | |||
|
Overall looks good to me. Just as I mentioned in #14612 (comment), the reason is we do not fix the versions of some binaries. We might need to revisit this after #14533 if resolved. |
See #14612 (comment)
Signed-off-by: spacewander [email protected]