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

Improve contributor experience with CI #13896

Open
serathius opened this issue Apr 6, 2022 · 11 comments · Fixed by #13956
Open

Improve contributor experience with CI #13896

serathius opened this issue Apr 6, 2022 · 11 comments · Fixed by #13956

Comments

@serathius
Copy link
Member

serathius commented Apr 6, 2022

  • 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

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.

@willbeason
Copy link
Contributor

  • 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

@ptabor
Copy link
Contributor

ptabor commented Apr 7, 2022

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

@willbeason
Copy link
Contributor

@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.

@vimalk78
Copy link
Contributor

@willbeason the PR #13956 should fix the issue you are facing

@serathius
Copy link
Member Author

Not all issues listed were addressed

@serathius serathius reopened this Apr 19, 2022
@stale
Copy link

stale bot commented Jul 31, 2022

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 stale bot added the stale label Jul 31, 2022
@serathius
Copy link
Member Author

serathius commented Apr 11, 2023

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.

@sharathsivakumar
Copy link
Contributor

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

@serathius
Copy link
Member Author

Sounds good @sharathsivakumar, thanks for looking into this!

@sharathsivakumar
Copy link
Contributor

@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.

@serathius
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

5 participants