etcd icon indicating copy to clipboard operation
etcd copied to clipboard

Improve contributor experience with CI

Open serathius opened this issue 3 years ago • 6 comments

  • In CONTRIBUTING.md, list scripts in scripts/ that a first-time contributor would need (and what they're for). Or - have a short explanation at the top of each script which briefly says what it's for. (Doesn't have to be a full usage guide, just "Updates license information in modules and submodules. CI checks that running this causes no changes." would go a long way)
  • For CI failures, suggest the appropriate script to fix the corresponding failure. For example, if the bom test fails, most likely the contributor just needs to run ./scripts/fix.sh

serathius avatar Apr 06 '22 15:04 serathius

  • Add instructions for running unit and integration tests locally. I didn't realize that just running go test ./... does not run all tests in the repository. Similarly ./scripts/test.sh surprisingly does not run the Go tests. The way to run all unit tests is PASSES='unit' ./scripts/test.sh
  • Add instructions for getting the same version of addlicense as the one in CI. For me, running addlicense locally fails, but CI passes. So the repository is correct, but I'm using the wrong version. Unfortunately addlicense does not have a subcommand that prints the version information, so it can't be automatically checked

willbeason avatar Apr 06 '22 16:04 willbeason

make test-smoke and make test-full are short cut's for O(1min) feedback and full feedback comparable with github presubmits respectively.

ptabor avatar Apr 07 '22 07:04 ptabor

@ptabor Thanks! For me make test-smoke fails because I seem to be using a different version of addlicense than the repository expects. It results in a usage error message from addlicense instead of it running as expected.

willbeason avatar Apr 07 '22 14:04 willbeason

@willbeason the PR https://github.com/etcd-io/etcd/pull/13956 should fix the issue you are facing

vimalk78 avatar Apr 18 '22 00:04 vimalk78

Not all issues listed were addressed

serathius avatar Apr 19 '22 13:04 serathius

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jul 31 '22 06:07 stale[bot]

With introduction of make test-* make verify-* and make fix-* I would propose to split ./scripts/test.sh to separate targets and document them all in makefile. Having a fix-* target for each verify-* should address the issue.

serathius avatar Apr 11 '23 07:04 serathius

Hey @serathius @jmhbnz Can you please assign me to this? I am keen to work on it.

With the example of make test-e2e, this currently calls scripts/test.sh as follows. PASSES="e2e" ./scripts/test.sh $(GO_TEST_FLAGS)

Are we proposing to split this individual target to a separate file called scripts/test-e2e.sh

sharathsivakumar avatar Sep 12 '23 19:09 sharathsivakumar

Sounds good @sharathsivakumar, thanks for looking into this!

serathius avatar Sep 13 '23 08:09 serathius

@serathius and @jmhbnz I have been away due to personal issues and could not complete this. However I am back now and I will get this done soon. I am looking to turn in the first PR by early next week. Hope that's fine.

sharathsivakumar avatar Nov 02 '23 07:11 sharathsivakumar

Not a problem, great to hear you are still interested. Awaiting your PR.

serathius avatar Nov 02 '23 09:11 serathius