-
-
Notifications
You must be signed in to change notification settings - Fork 150
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 with custom git version #330
Conversation
That is such a good idea. I love it. |
I would rather reduce the version number until the pipeline breaks to learn what our current min is. Then we can decide what to do. Relying on the maintenance level of git is understandable, but who cares about whether their installed git version is maintained? |
TLDR: ubuntu-latest with git 2.20.5 worksubuntu-latest with git 2.19.6 failssee #331 ubuntu-latest with git 2.19.6 [without squash-wip] worksubuntu-latest with git 2.18.5 [without squash-wip] worksubuntu-latest with git 2.17.6 [without squash-wip] worksubuntu-latest with git 2.16.6 [without squash-wip] worksubuntu-latest with git 2.15.4 [without squash-wip] worksubuntu-latest with git 2.14.6 [without squash-wip] worksubuntu-latest with git 2.13.7 [without squash-wip] worksubuntu-latest with git 2.13.0 [without squash-wip] worksubuntu-latest with git 2.12.5 [without squash-wip] fails
macos-latest with 2.20.5 fails:It fails to install the git version and does't even get to the tests
|
a74cc42
to
12b53a6
Compare
Ok, so we are now testing what we can. |
- name: Use Go 1.16.x | ||
uses: actions/setup-go@v2 | ||
with: | ||
go-version: '~1.16.0' | ||
- name: Test | ||
run: go test | ||
run: go test -test.v |
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.
@gregorriegler do we need that -test.v flag? Or was that just for testing which version works and which not?
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 it so I can see the logs, because I didn't have any Feedback. And I think we should now keep it. The tests are not very useful if they don't tell you why they fail.
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.
But I agree, we should not need to use this flag in order to get the Feedback we need. I will create an Issue.
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.
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.
fine for me.
@gregorriegler I added one comment. Everything else looks fine to me. |
@@ -3,16 +3,39 @@ name: Test | |||
jobs: | |||
test: | |||
strategy: | |||
fail-fast: false |
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.
@gregorriegler I would remove that. We should fail fast in my opinion
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.
Why would you want it? Are we not failing fast?
Let me tell you why I turned it off. So it was turned on, and what happened was that when one build failed, I didn't get to see the other builds finish. It all suddenly aborted. So I didn't get the feedback where the other builds would have failed and why.
It looks like a good way to save hardware resources. On the other hand I don't understand in what way we're not failing fast, if we keep running the other builds after one of them failed. We did see it fail, so we fail as soon as possible, even with the flag set to false, no?.
On python approvals for example, we also use this flag: https://github.com/approvals/ApprovalTests.Python/blob/ee6312d8848e7ee13e8bb5bafbe96bcd7053aa7a/.github/workflows/test.yml#L10
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 did some research again and github actions seem to be free for all public projects without any limit so this is fine for me.
I adjusted the CI pipeline, such that it can be tested with different git versions. I would propose to go with latest and with the minimum git version we want to support. I do not know if we somewhere tell the user a minimum git version. Which version should we add here as minimum version @simonharrer @gregorriegler ?