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

Allow git-lfs-transfer integration tests to be skipped #4677

Merged
merged 1 commit into from
Oct 12, 2021

Conversation

fh1ch
Copy link
Contributor

@fh1ch fh1ch commented Oct 8, 2021

This PR introduces the new environment variable TEST_SKIP_LFS_TRANSFER which offers the possibility to skip git-lfs-transfer tests based on scutiger-lfs while also running within CI:

make integration TEST_SKIP_LFS_TRANSFER=true

This is required since it's currently not possible to disable the use of git-lfs-transfer when running within a CI build (detected automatically via CI variable). Since git-lfs-transfer has to be installed on a systems via other means, most external CI builds will fail as they can't use the git-lfs Docker test image, which preinstalls git-lfs-transfer.

This is patch needed to make git-lfs packaging work for Linux distributions like Alpine Linux (e.g. see https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/26213).

An alternative to the approach used within this PR would be to change the logic to a positive one, meaning replacing the current $CI && !$TEST_SKIP_LFS_TRANSFER with an e.g. $USE_GIT_LFS_TRANSFER one. This would disable those tests on all environments, allowing the git-lfs CI test to explicitly enable them.

/cc @bk2204

Copy link
Member

@bk2204 bk2204 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey,

My preference here is that we do either one of two things:

  • Not set CI in environments other than the Git LFS CI, which I think makes things more robust.
  • Name the new environment variable starting with TEST (e.g., TEST_SKIP_LFS_TRANSFER), which is what our other tests do.

@fh1ch fh1ch force-pushed the test/testhelpers-skip-git-lfs-transfer branch from e972403 to cea45a5 Compare October 12, 2021 18:33
@fh1ch
Copy link
Contributor Author

fh1ch commented Oct 12, 2021

@bk2204 thanks a lot for the feedback. Not setting CI is not really feasible IMO, as this is automatically done by pretty much every CI system like GitHub Action, GitLab CI, Jenkins, ... It would require the creation a completely empty env via other means.

I really like the TEST_SKIP_LFS_TRANSFER name change tough, especially since there already seems to be an established practise. I just reworked the MR to that, let me know what you think.

Copy link
Member

@bk2204 bk2204 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Once CI is green, I'll merge this.

@bk2204 bk2204 merged commit 0475561 into git-lfs:main Oct 12, 2021
bk2204 added a commit to bk2204/git-lfs that referenced this pull request Oct 25, 2021
…lfs-transfer

Allow git-lfs-transfer integration tests to be skipped
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants