Accept git 1.7.2 as the minimum version #90
Conversation
|
Debian 6 is EOL so I'd say that a very poor argument for lowering the minimum requirement. And I'd require tests that proves that everything we use actually works in the specified version if we are to lower it. Installing a specific version of Git in a docker-container for testing is fairly straight forward |
|
You'd have real-world testing as we'd be installing it on a Debian 6,
whereas the blocker is exactly this issue (we're staying with Gogs
until then).
|
|
There, with latest commit the tests are also run with git-1.7.0 installed. And they pass. |
|
Thanks to some research conducted by @bkcsoft and someone else whose name I don't know (coming from IRC) it was found that: git.GetLatestCommitTime and git.CommitCount both use the Workarounds for older versions were removed with commits like this: So before accepting this PR we'd need:
|
|
More findings: |
The test checks that latest commit time is before now and more recent than the commit this PR is based at Test no error is raised by time parsing and GetLatestCommitTime Print actual time when tests fail
Debian old old (very old) distribution (6.0 aka Squeeze) ships version 1.7.10.4. The version requirement was raised in gogs#46 supposedly for the need of "symbolic-ref" command, but that command is supported by the 1.7.2 version too, and possibly even older versions.
strk
changed the title
|
Drone testing is now working, and showed that 1.7.0 would not really work with CommitsCount method, but also shows that 1.7.2 is fine, so I've updated the PR title and requirement to be 1.7.2 instead of 1.7.0 -- I'm still happy because Debian 6.0 is shipping 1.7.10 ... Any other method you'd think as suspicious, that needs testing ? Or I'd merge this PR and react to whatever issue comes out as more tests are added... |
|
Could you add individual drone testing steps for latest git (as it was before) and this step that tests with 1.7.2 as new step? |
|
it is already in two steps, you can see a |
Reduce steps, concatenating them in logical steps
|
Now that you have cleaned output it's much better now. LGTM |
.drone.yml
Outdated
| # Test with latest git version | ||
| - git --version && make test | ||
| # Test with git version 1.7.2 | ||
| - apk add --update autoconf zlib-dev > /dev/null && |
There was a problem hiding this comment.
This is IMHO a no-go. This will compile git for EVERY build run. Please provide separate docker images.
There was a problem hiding this comment.
do we have an official place to store such image ?
There was a problem hiding this comment.
I would suggest somebody pushs it to something like gitea/ci:git-1.7 and gitea/ci:git-1.8
There was a problem hiding this comment.
Ok I've added a docker/ci/Dockerfile-git-1.7 and a Makefile in that directory to build it, next I'd need permissions to push onto gitea/ namespace on docker hub, can you grant me that ? Or push it yourself ?
There was a problem hiding this comment.
I'm "strk" on docker hub if anyone can give me perms to upload the new image
There was a problem hiding this comment.
Well I've uploaded to my own registry and updated .drone.yml accordingly. Can you now approve the PR ?
|
centos 6 seems to have 1.7.1 |
|
I tried 1.7.1 but it failed one test. I'm happy if you want to implement a workaround so we can still support 1.7.0 too |
|
@strk I don't think increasing code complexity is worth the gain |
|
please follow @tboerger 's advice otherwise LGTM |
|
@tboerger need your approval |
| - make build | ||
| testing-git-latest: |
There was a problem hiding this comment.
It seems this section is unnecessary.
There was a problem hiding this comment.
Why do you think so ? It is the section that tests with latest git version (2.something)
There was a problem hiding this comment.
test-general section should be the latest git? So that a new section is unnecessary.
There was a problem hiding this comment.
test-general does not run make test, only subsequent sections do
There was a problem hiding this comment.
it not run make test because you remove it. Pick it back and remove section testing-git-latest is OK.
| image: webhippie/golang:edge | ||
| pull: true | ||
| commands: | ||
| - git update-ref refs/heads/test HEAD |
|
Why update-ref is needed?
Because one of the tests expects an head ref to be defined
in the repository used as target, and we're using the
code repository as test target.
|
|
Than probably also for testing-1-7 you should add this command so that if set that tests run in parallel they would not fail if run in different order |
|
Done with 9c1fd83 |
|
@tboerger your approval is the last missing bit here |
|
I give up. This nitpicking makes contributors run away.
If you have strong opinions about how to do things just do them.
I can't spend all my time moving bits around, this PR adds testing
for two git versions, using my own space for serving the docker
and we're still discussing about naming/commenting sections?
Please stop this torture
|
|
Wouldn't it be simpler to add an environment variable that allows users to just ignore the "Git version not supported" message? |
|
Wouldn't it be simpler to add an environment variable that allows
users to just ignore the "Git version not supported" message?
This PR contains 2 unit tests for 2 methods, and support
for having Done run all tests with git 1.7.2 (in addition to
the current latest git version).
It's done already. What's simpler than pushing the "merge" button ?
As per older git versions: 1.7.1 was found to be unsupported so
I don't see it as a good service to continue running with
a known broken setup.
|
|
@strk Please send a PR to gitea to update git. |
|
Thanks. Gitea PR:
go-gitea/gitea#3011
|
Debian old old (very old) distribution (6.0 aka Squeeze)
ships version 1.7.10.4.
The version requirement was raised in #46 supposedly for the
need of "symbolic-ref" command, but that command is supported
by the 1.7 version too, and even older versions.