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

Unable to pull latest changes for this repo using go mod tooling #47

Closed
aminjam opened this issue Sep 21, 2021 · 21 comments
Closed

Unable to pull latest changes for this repo using go mod tooling #47

aminjam opened this issue Sep 21, 2021 · 21 comments

Comments

@aminjam
Copy link
Contributor

aminjam commented Sep 21, 2021

Context

This repository has published 3 tags in the past. Some of the tags that were published previously don't belong to any commit on the main repository. Currently, commits are made to the master branch of this repository and we don't create new tags. All of the tags were created prior to go having any supported process for go modules. Since the introduction of the go modules, when a user depends on code.cloudfoundry.org/larger as a dependency, it actually pulls in v2.0.0 by default since go automatically thinks that a tagged version of the any dependency is more stable than the main/master branch. In order to pull in the latest from master, we have to use a replace directive for the latest commit and wouldn't be able to update that SHA with the default go tooling.

Proposal

  • Remove tags from this repository
  • Update README with SHAs that were previously linked to the tags, in case someone in the community have to use older versions of this repo. They can then use a replace directive with a given SHA to make sure they are pulling in the same version.
  • When a user runs go mod tidy & go mod vendor for a repository, they should be able to fetch the latest changes for code.cloudfoundry.org/lager
  • Update runtime repositories to remove the replace directive.

I propose to keep this open for the next 2 weeks and discuss any other solutions/gotchas/concerns.

cc @cloudfoundry/runtime

@aminjam aminjam changed the title Remove published tags from this repo Unable to pull latest changes for this repo using go mod tooling Sep 21, 2021
@Benjamintf1
Copy link
Member

you could just add a v2.0.1 tag. I don't think this is likely to change(at all even) so it's not likely to be a long term concerns. Also, IIRC, various things might be caching older tags in various ways anyways, which is to say, it might not be a good idea to delete.

silvestre added a commit to cloudfoundry/app-autoscaler-release that referenced this issue Jan 11, 2023
The `lager` repo has outdated tags defined, which override the current
versions:

> This repository has published 3 tags in the past. Some of the tags
> that were published previously don't belong to any commit on the main
> repository. Currently, commits are made to the master branch of this
> repository and we don't create new tags. All of the tags were created
> prior to go having any supported process for go modules. Since the
> introduction of the go modules, when a user depends on
> code.cloudfoundry.org/larger as a dependency, it actually pulls in
> v2.0.0 by default since go automatically thinks that a tagged version
> of the any dependency is more stable than the main/master branch. In
> order to pull in the latest from master, we have to use a replace
> directive for the latest commit and wouldn't be able to update that
> SHA with the default go tooling.

cloudfoundry/lager#47
@silvestre
Copy link
Member

Dear @cloudfoundry/wg-app-runtime-platform-logging-and-metrics-approvers

could this issue be reexamined?

The requirement to add a replace directive to get the current version is rather cumbersome.

Thanks!

app-autoscaler-ci-bot pushed a commit to cloudfoundry/app-autoscaler-release that referenced this issue Jan 11, 2023
…#1270)

* fix(deps): update module github.com/pivotal-cf/brokerapi/v8 to v8.2.3

* Add replace for `lager` to ignore outdated tags

The `lager` repo has outdated tags defined, which override the current
versions:

> This repository has published 3 tags in the past. Some of the tags
> that were published previously don't belong to any commit on the main
> repository. Currently, commits are made to the master branch of this
> repository and we don't create new tags. All of the tags were created
> prior to go having any supported process for go modules. Since the
> introduction of the go modules, when a user depends on
> code.cloudfoundry.org/larger as a dependency, it actually pulls in
> v2.0.0 by default since go automatically thinks that a tagged version
> of the any dependency is more stable than the main/master branch. In
> order to pull in the latest from master, we have to use a replace
> directive for the latest commit and wouldn't be able to update that
> SHA with the default go tooling.

cloudfoundry/lager#47

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Silvestre Zabala <silvestre.zabala@sap.com>
@ctlong
Copy link
Member

ctlong commented Jan 11, 2023

@silvestre this issue was discussed at a recent ARP WG meeting. The upshot of it was that I (and other approvers) have the go-ahead to complete this plan.

I plan to get to this by the end of the week. Hopefully that's a soon enough timeline for you 😄

@Benjamintf1
Copy link
Member

https://github.com/cloudfoundry/lager/tree/v3.0.0

Created a new release.

@silvestre
Copy link
Member

@ctlong @Benjamintf1 Awesome, thanks! ❤️

Now to update everyone to v3. e.g. pivotal-cf/brokerapi#218 :)

@winkingturtle-vmw
Copy link
Contributor

@ctlong @Benjamintf1 Can you please provide some context on why we decided to cut a v3 instead of removing all of the tags form this repo as suggested in this issue?

The problem with the above solution is that any update to this repo requires releasing a new tag even if this update is just a dependabot bump. This feels very unsustainable for a widely used repository.

@ctlong
Copy link
Member

ctlong commented Mar 16, 2023

@winkingturtle-vmw we didn't want to go down a path of removing all tags / published versions because we need new releases in order for dependabot to bump lager in modules that use it. While it does add more toil to the maintainers of this repo, it makes life a lot easier for the maintainers of modules that import it, which seemed a fair trade for a widely used repository.

My personal preference was to retract enough releases to get us back to a v0.X.X state so that we could automate cutting minor versions every couple weeks without needing to worry about the promise of backward compatibility. However, research into retracting releases indicated the go community in general does not like retractions and isn't well set up for them. The recommended process would be to keep the tags on the repo and publish a retraction. We discarded that approach because it creates a difference between the repo tags and https://pkg.go.dev/, which could lead to confusion.

Ultimately, when looking at cutting a new releases we didn't want to go through years of commits to determine whether we wanted to cut a minor or major version here, so we just cut a major release to start fresh.

@winkingturtle-vmw
Copy link
Contributor

winkingturtle-vmw commented Mar 16, 2023

it makes life a lot easier for the maintainers of modules that import it, which seemed a fair trade for a widely used repository.

@ctlong can you help me understand how this is easier to import new updates to this repo, unless we continuously tag v3.x.x (at some arbitrary interval), I don't how anyone can get updates from this repo.

lager is more of an internal library that is backward compatible and will remain so for it's lifetime, so versioning this repo has no meaning.

However, research into retracting releases indicated the go community in general does not like retractions and isn't well set up for them.

Not sure I understand this. We don't have releases for this repo, so tagging is arbitrary. Versioning repos makes sense when there is a real change that's taking place e.g. ginkgo v2 where we are deprecating some things and introducing new ones. This will not be the case for lager. On the downside, there are roughly 425 packages that import this repo just in code.cloudfoundry.org. that needs to be updated in order to see any changes.

I discovered this issue when someone wanted to update lager to v3 in garden, which then impacted

  • code.cloudfoundry.org/guardian in 150 files
  • code.cloudfoundry.org/debugsever
  • and probably more but I stopped after that

Additionally, guardian uses github.com/st3v/glager which then imports code.cloudfoundry.org/lager, so unless that user (st3v) decided to update import path V3, we would not be able to use this library and have to change implementation in order to stay up-to-date with this repository.

I propose that we still go back to the original plan and remove v1,v2,v3 tags, so that everyone can get updates for this repo when we merge PRs. In other words, we are making it simpler for maintainers as well as users of this repo without requiring 425 other repos to update their references.

@ctlong
Copy link
Member

ctlong commented Mar 17, 2023

@winkingturtle-vmw I don't think it's a good idea to remove the tags on this repository. Golang treats tagged code as a version to make available for import, and explicitly tells us to "not [to] delete version tags from your repo... or overwrite [them]." Removing the tags could, in the worst-case, completely break current and future imports of this code.

Given that, I think we need to continue cutting releases for this code. When people look to import this code and see that there are releases it's fair to assume that they will look to import a release rather than the latest commit on master. So we should look to cut releases for this code at semi-regular intervals to incorporate certain bug fixes, feature improvements, or other changes that may come up, as we do with other Golang code. I agree that's not something we've been great at in the past, but it seems like a worthy goal to pursue.

@winkingturtle-vmw
Copy link
Contributor

@ctlong I don't think the current solution of renaming the package import with a v3 tag is ideal. It's essentially breaking imports of every single library that has imported this library. In the example above, we simply can't ask every single library to update their import path. I think that at the very least we should remove the v3 from the import path and notify the 19 users to change the import path and keep the original path.

@Benjamintf1
Copy link
Member

Benjamintf1 commented Mar 20, 2023

package imports with major version in the import path are the default assumption for golang these days, not a strange and unexpected change.

@winkingturtle-vmw
Copy link
Contributor

package imports with major version in the import path are the default assumption for golang these days, not a strange and unexpected change.

@Benjamintf1 @ctlong I am not sure if it is clear or not. I am not referring to the fact that the change is strange or unexpected, it's breaking upgrade path for 425 packages where it doesn't need to and we don't have control over many of those packages. There was no breaking change here to require v3 in the import path.

@ctlong
Copy link
Member

ctlong commented Mar 20, 2023

@winkingturtle-vmw you may be right that there was no breaking change between v2 and v3, but the import path was going to have to change regardless. The latest release prior to v3.0.0 was v2.0.0 in 2018. However, the import path was never changed to match that tag, which is against Golang conventions where "a new major version of a module must have a different module path than the previous version."

There was then no benefit to reading through 4+ years of commits that we didn't have context on to determine whether a breaking change had been made or not. In either case, the import path would have to changed, which would necessitate all imports to be updated in order to stay up-to-date.

@winkingturtle-vmw
Copy link
Contributor

However, the import path was never changed to match that tag, which is against Golang conventions where "a new major version of a module must have a different module path than the previous version."

I am not sure if I agree with this. Convention only matters when there is a guideline in place at the time of development. go.mod was first officially supported in Go 1.13 in Sep 2019. v2 was tagged in Oct 2018 which predates "convention" by a whole year.

Additionally there is a total of ~100 commits on this repo overall where the v2 was only 6-7 commits away from the time this issue was opened.

In either case, the import path would have to changed, which would necessitate all imports to be updated in order to stay up-to-date.

I still don't understand why this had to be the case. go get -u code.cloudfoundry.org/lager should've/will probably still pull in new v3 tags, regardless of what Go conventions are. Convention in the case of Go matters when there is a real breaking change, not in the case of arbitrary tagging a repository. The recommendation is in place so that when a user updates their dependencies, they don't experience breaking change in APIs. Is there something technical that I am missing here? @ctlong

@Benjamintf1
Copy link
Member

Go mod is the officially supported convention since 2019. There's no excuse for us to not be supporting it correctly in this day and age.

v2 was tagged in Oct 2018 which predates "convention" by a whole year.
Which is part of the reason we updated the library to v2. If a user has not updated to follow convention, they should be able to continue to use v2 until they update their library/program/whatnot to v3.

Convention in the case of Go matters when there is a real breaking change, not in the case of arbitrary tagging a repository <- repository tags are not arbitrary, they're part of the golang expectations.

Being able to have multiple incompatible versions of a library at the same time, yes, is a reason why golang has pushed for major version versioning. This is enforced via your dependency library(aka gomod, the official dependency management for golang, and the thing we, as cloudfoundry, use to maintain golang dependencies).

@ctlong
Copy link
Member

ctlong commented Mar 20, 2023

@winkingturtle-vmw it seems like we may just have very different opinions about the "right" way to manage this repo. Since y'all are likely to take over the repo soon, you're welcome to determine your own path. My personal opinion is still in favor of keeping the v3 bump and moving forward from there, but it's true I wouldn't be the person to have to change most of the module paths in that case.

If you want to chat more about this maybe we should hop on a zoom eventually? No pressure though.

@winkingturtle-vmw
Copy link
Contributor

If a user has not updated to follow convention, they should be able to continue to use v2 until they update their library/program/whatnot to v3.

Like I said many times before, this is not possible. When one consumer is updated, the rest has to follow. Please see this comment.

repository tags are not arbitrary, they're part of the golang expectations.

It is arbitrary. See comment

lager is a library that get little/no change at all in it's interface. That's the reason why this is imported everywhere. My understanding from your reply is that the decision to change the import path had no technical reasons and it could've remained the same import path and fetched v3 with the same import path.

The problem we have at hand right now is whether we are going to update over 400 packages (and more) in order to be able to use the latest commit or revert the package name so it continues to be imported the same way. Since the API hasn't changed and there isn't any changes that's backward incompatible from v2 (which is about a handful of commits behind), the import path should remain the same IMO.

I am happy to have a zoom conversation too if needed, so that I can help translate the problem with changing import paths and downstream consumers. I have seen this happen in other repos under logging-and-metrics (e.g. go-log-cache where there doesn't seem to be any breaking-changes. This is causing every downstream consumer stop upgrading unless every single consumer plays nice and upgrade at the same time. This problem is multiplied with lager because everyone uses it where go-log-cache might have much smaller audience.

@Benjamintf1
Copy link
Member

Using correct go mod versioning, in the long run, makes things easier, not harder. At the moment, any breaking or unintended change would ripple to any and every user unless they all cooperate on versioning all the time. Using go.mod versioning allows library maintainers to use different versions of a library and upgrade when appropriate.

The versioning and managing of the deigo-release repository, is it's own problem. I've offered multiple times to help with it in the past, and even at points(although they are outdated), got pull request paths started, and was willing to work through the implications.

Moving on to go mod conventions will help contributors. It will help a lot as we start to adopt and use tools like dependabot, which expect these conventions to be followed.

But I digress. We don't have to as a working group(the observability section), manage lager. And at the end of the day, we should probubly be migrating to the new base golang structured logger rather then maintaining our own at some point. But I would highly recommend following modern golang conventions as the less we do, the more hostile we make things for ourselves and for new contributors/etc.

@Benjamintf1
Copy link
Member

It's maybe possible to use v2 and v3 at the same time, I believe, because v3 uses correct go mod versioning, but I'm not 100% certain on that.

@ctlong
Copy link
Member

ctlong commented Mar 20, 2023

Interesting side-note: structured logging is coming to the standard library as soon as Go 1.21.

@winkingturtle-vmw
Copy link
Contributor

I created two test repositories that shows the migration path.

1️⃣ a Library that is supposed to show lager’s issue (https://github.com/winkingturtle-vmw/go-tagging)
2️⃣ a Client that is supposed to show what would happen when import path has been removed (https://github.com/winkingturtle-vmw/go-tagging-client)

With the following test, I feel pretty confident that folks can still use in code.cloudfoundry.org/lager/v3 importpath as long as we cut a new tag. This means that if we cut a new v4 tag for lager, v3 import paths would still work and remain backward compatible while any new tags can be fetched under the old path.

geofffranks pushed a commit that referenced this issue Mar 21, 2023
Context: #47

Signed-off-by: Geoff Franks <gfranks@vmware.com>
geofffranks pushed a commit that referenced this issue Mar 21, 2023
Context: #47

Signed-off-by: Geoff Franks <gfranks@vmware.com>
@aminjam aminjam closed this as completed Sep 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

5 participants