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

Fetch git tags by default #1537

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lox
Copy link
Contributor

@lox lox commented Nov 24, 2021

Historically this wasn't added due to --tags not being supported in earlier git versions (#338), but that was 2016. By now it's well supported and optimized so it's quite fast.

I'd advocate for this change being the least surprising, and adding it to defaults shouldn't break any existing configurations.

Historically this wasn't added due to --tags not being supported in
earlier git versions, but that was 2016. By now it's well supported
and optimized so it's quite fast.

I'd advocate for this change being the least surprising, and adding
it to defaults shouldn't break any existing configurations.
@keithduncan
Copy link
Contributor

keithduncan commented Nov 24, 2021

It would be good to codify the minimum required git version for the agent.

Is there a way to feature detect whether the git CLI or remote server supports this and degrade gracefully?

@jradtilbrook
Copy link
Member

jradtilbrook commented Nov 25, 2021

I know some people operate by tagging every commit to the default branch (not sure the reasoning but I know it happens). So I think this could cause performance issues for that sort of thing. Would it be safer to default to off? Though if that's the case, there's almost no point in this change I suppose.
Also, I wonder what the coverage of this change would be - are enough people building on or doing things with tags to make it useful or will it hurt more than it'll help?

Full disclosure: I know nothing about the performance of fetching tags or its recent improvements

@lox
Copy link
Contributor Author

lox commented Nov 25, 2021

I'd argue this is the sensible default and that folks with weird workflows should opt-out of it.

@lox
Copy link
Contributor Author

lox commented Nov 25, 2021

Version detection with git sounds like a good thing to have @keithduncan.

@lox
Copy link
Contributor Author

lox commented Nov 25, 2021

So I think this could cause performance issues for that sort of thing.

@jradtilbrook given the commit object is already being pulled down, is there reason to think adding a tag ref would incur substantial performance penalties? Looking at the git changelog, there has been lots of work done to make it quick.

@jradtilbrook
Copy link
Member

@lox I honestly don't know. I was just contributing a thought I had. It's quite possible that it won't - especially since the default is not to perform clean checkouts (unless its a new agent/instance) so pulling a few more commits to what is already there along with their tags is probably no big deal

@lox
Copy link
Contributor Author

lox commented Nov 25, 2021

@jradtilbrook yeah honestly I’m not sure either!

@lox
Copy link
Contributor Author

lox commented Dec 5, 2021

Without this behaviour in place, I've experienced users frequently running into weird race conditions where the latest reachable tags are based on which build on the host has fetched tags for that repo. It creates weirdly unreproducible bugs for anything that uses tags (for instance goreleaser or any manner of versioning tool), which is a bummer. The answer is always "Oh! Add a git fetch --tags".

It's definitely possible this could break things, but would be awesome to consider for a v4.0.0 release as I think it's a good default.

@moskyb moskyb added v4 Breaking changes that will be included in Agent v4 breaking Changes to existing behaviour users might rely on labels Apr 28, 2022
icehaunter added a commit to electric-sql/vaxine that referenced this pull request Feb 1, 2023
icehaunter added a commit to electric-sql/electric that referenced this pull request Feb 1, 2023
icehaunter added a commit to electric-sql/electric that referenced this pull request Feb 7, 2023
icehaunter added a commit to electric-sql/electric that referenced this pull request Mar 2, 2023
@lox
Copy link
Contributor Author

lox commented Jun 1, 2023

After living with this change for a year or so, I will say it makes build output very verbose with lots of tags fetched each time 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Changes to existing behaviour users might rely on v4 Breaking changes that will be included in Agent v4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants