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

vendor: Pin github.com/optiopay/kafka to commit before fork #15159

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

christarazi
Copy link
Member

This dependency has been causing a lot of issues for those who are
consuming the Cilium code as a module. It required consumers to
duplicate the replace directive for optiopay/kafka in their
respective go.mod file.

This commit fixes this by pinning the original optiopay/kafka to the
commit when it was forked by Cilium developers (and hence the replace
directive).

Suggested-by: Glib Smaga code@gsmaga.com
Signed-off-by: Chris Tarazi chris@isovalent.com

This dependency has been causing a lot of issues for those who are
consuming the Cilium code as a module. It required consumers to
duplicate the `replace` directive for `optiopay/kafka` in their
respective go.mod file.

This commit fixes this by pinning the original `optiopay/kafka` to the
commit when it was forked by Cilium developers (and hence the `replace`
directive).

Suggested-by: Glib Smaga <code@gsmaga.com>
Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi added release-note/misc This PR makes changes that have no direct user impact. area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. labels Mar 2, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 2, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Mar 2, 2021
@christarazi
Copy link
Member Author

If this fixes it, then we should update the docs and references such as https://github.com/cilium/client-examples.

@christarazi christarazi marked this pull request as ready for review March 3, 2021 22:30
@christarazi christarazi requested a review from a team as a code owner March 3, 2021 22:30
@christarazi christarazi requested a review from glibsm March 3, 2021 22:30
@christarazi
Copy link
Member Author

Here's how it was tested:

❯ cat --plain main.go go.mod
package main

import (
    "fmt"

    "github.com/cilium/cilium/pkg/hubble/observer/observeroption"
)

func main() {
    var opt observeroption.Option
    fmt.Println(opt)
}
module f

go 1.16

replace github.com/cilium/cilium => /home/chris/code/cilium/cilium

require github.com/cilium/cilium v0.0.0-00010101000000-000000000000
/tmp/f
❯ go mod tidy -v
go: downloading github.com/sirupsen/logrus v1.7.0
...
go: downloading golang.org/x/mod v0.3.0

/tmp/f 9s
❯ go clean -modcache ./

# Switched the repo back to master

/tmp/f
❯ go mod tidy -v
go: github.com/cilium/cilium@v0.0.0-00010101000000-000000000000 requires
        github.com/optiopay/kafka@v0.0.0-00010101000000-000000000000: invalid version: unknown revision 000000000000

@tklauser tklauser added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 4, 2021
@tklauser
Copy link
Member

tklauser commented Mar 4, 2021

Changes in go.mod only with no effects on vendor/, thus no need to run full CI. Marked as ready to merge.

@aanm aanm merged commit adb941b into cilium:master Mar 4, 2021
1.10.0 automation moved this from In progress to Done Mar 4, 2021
@christarazi christarazi deleted the pr/christarazi/fix-kafka-dep branch March 4, 2021 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants