-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
✨ (go/v4) Add GitHub Actions #4110
✨ (go/v4) Add GitHub Actions #4110
Conversation
ba717f3
to
2b58377
Compare
b263120
to
aba4985
Compare
b8fb2aa
to
77decdb
Compare
77decdb
to
b9dbd36
Compare
/hold cancel |
@@ -161,6 +162,9 @@ func (s *initScaffolder) Scaffold() error { | |||
&templates.Golangci{}, | |||
&e2e.Test{}, | |||
&e2e.SuiteTest{}, | |||
&github.E2eTestCi{}, | |||
&github.TestCi{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamacedo86 Everything looks good to me. Just one question: Is it necessary to maintain separate files for each of GH Action template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to set up 3 separate jobs as follows:
![Screenshot 2024-09-03 at 18 53 41](https://private-user-images.githubusercontent.com/7708031/364088435-822fa446-e294-4cbd-ba33-ce17b95dccd6.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkxMTY0NDMsIm5iZiI6MTczOTExNjE0MywicGF0aCI6Ii83NzA4MDMxLzM2NDA4ODQzNS04MjJmYTQ0Ni1lMjk0LTRjYmQtYmEzMy1jZTE3Yjk1ZGNjZDYucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDlUMTU0OTAzWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9N2VmOGZmMTJlZmRiYWNjNDZlMjhhZDQzYzg0NWM5MjI2MTE4NzEzYWU0MDExYWI4ZWIyZmJkODMxZWNhMDcxYyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.lhxan0geFYjKyTW3n0E9jnS744gBeXBWK-TiYtih1dc)
Each job will have its own file so that if a failure occurs, it's easy to identify which target/job/test failed. Running the jobs in parallel will also make the process faster and easier to manage.
This separation also makes it clear which file corresponds to each task: test.yml for tests, lint.yml for linting, and so on. This approach simplifies any future changes and helps maintain clarity—if everything were in the same file, it would be more challenging to pinpoint issues and make updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamacedo86 makes sense to maintain the separation of concern. Everything else seems good for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! 🚀
- name: Run linter | ||
uses: golangci/golangci-lint-action@v6 | ||
with: | ||
version: v1.59 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: what about using the latest release v1.61.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the same version that we are using.
so we can upgrade all by once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
|
||
- name: Running Test e2e | ||
run: | | ||
go mod tidy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: looks like an additional indent here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets fix this one before get merged
Good catcher !!
/hold
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, Kavinjsir, TAM360 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add GitHub actions to the default scaffold. Motivation: - help developers use the tests in the CI - make clear the purpose of the e2e tests scaffolds - encourage good practices and standard approachs
b9dbd36
to
5a69aac
Compare
/hold cancel Now it is good to fly 🎉 |
Within the changes of this PR new projects will become to be scaffold with GitHub actions which will allow users be aware of how to test their changes on the ci.
Either will make clear the purpose of the test-e2e and encorage common/best practices to ensure the quality of the properties
Closes: #4088