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

contrib/go-chi: remove chi.v4 integration #1233

Merged
merged 1 commit into from
Apr 4, 2022

Conversation

Julio-Guerra
Copy link
Contributor

@Julio-Guerra Julio-Guerra commented Apr 4, 2022

This integration was recently integrated but is not actually not being used.
We are removing it in order to avoid go.mod issues due to the dependencies
of chi v4 and that won't be fixed since it is no longer maintained.

Note that the chi v5 middleware works with the v4 API so chi v4 users can
still use our v5 integration.

Fixes #1220

This integration was recently integrated but is not actually not being used.
We are removing it in order to avoid go.mod issues due to the dependencies
of chi v4 and that won't be fixed since it is no longer maintained.

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# HEAD detached at origin/v1
# Changes to be committed:
#	deleted:    contrib/go-chi/chi.v4/appsec.go
#	deleted:    contrib/go-chi/chi.v4/chi.go
#	deleted:    contrib/go-chi/chi.v4/chi_test.go
#	deleted:    contrib/go-chi/chi.v4/example_test.go
#	deleted:    contrib/go-chi/chi.v4/option.go
#	modified:   go.mod
#	modified:   go.sum
#
# Untracked files:
#	internal/appsec/waf/wasm/
#
@Julio-Guerra Julio-Guerra requested a review from a team as a code owner April 4, 2022 13:17
@Julio-Guerra Julio-Guerra requested a review from a team April 4, 2022 13:17
@Julio-Guerra Julio-Guerra requested a review from a team as a code owner April 4, 2022 13:17
@Julio-Guerra Julio-Guerra added the bug unintended behavior that has to be fixed label Apr 4, 2022
@Julio-Guerra Julio-Guerra added this to the 1.38.0 milestone Apr 4, 2022
Copy link
Contributor

@nsrip-dd nsrip-dd left a comment

Choose a reason for hiding this comment

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

Would we consider this a breaking change since the code is completely removed? Or are we pretty sure nobody was actually using this integration?

Also I see lots of stuff removed from the go.mod file. Which Go version did you run go mod tidy with? As far as I'm aware it shouldn't matter too much which but I'm just curious.

@Julio-Guerra
Copy link
Contributor Author

@nsrip-dd, yes, this is a breaking change indeed. The only other option I see is making it a submodule, which is also a breaking change in the end since the import path would be different.

I used go1.12 mod tidy here - go1.14 was failing at removing them in this case.
The more I update the version I use for go mod tidy, the more it removes go.mod indirect entries.

The verbose mode with -v says it's because they are unused and I grep'd a few of them which are indeed not used:

$ ~/go/go1.12/bin/go mod tidy -v
unused github.com/Azure/go-ansiterm
unused github.com/Microsoft/hcsshim
unused github.com/PuerkitoBio/goquery
unused github.com/alecthomas/template
unused github.com/alecthomas/units
unused github.com/armon/go-radix
unused github.com/aws/aws-sdk-go-v2/service/sso
unused github.com/beorn7/perks
unused github.com/cenkalti/backoff/v3
unused github.com/cncf/udpa/go
unused github.com/cncf/xds/go
unused github.com/containerd/containerd
unused github.com/containerd/continuity
unused github.com/coreos/go-systemd
unused github.com/docker/distribution
unused github.com/docker/docker
unused github.com/docker/go-connections
unused github.com/envoyproxy/go-control-plane
unused github.com/evanphx/json-patch/v5
unused github.com/fatih/structs
unused github.com/go-chi/chi/v4
unused github.com/go-gl/glfw
unused github.com/go-ini/ini
unused github.com/go-kit/kit
unused github.com/go-ldap/ldap
unused github.com/go-ldap/ldap/v3
unused github.com/golang-jwt/jwt
unused github.com/golang/mock
unused github.com/google/martian/v3
unused github.com/grpc-ecosystem/grpc-gateway
unused github.com/hashicorp/consul/sdk
unused github.com/hashicorp/go-kms-wrapping/entropy
unused github.com/hashicorp/go-plugin
unused github.com/hashicorp/go-secure-stdlib/base62
unused github.com/hashicorp/go-secure-stdlib/mlock
unused github.com/hashicorp/go-secure-stdlib/password
unused github.com/hashicorp/go-secure-stdlib/tlsutil
unused github.com/hashicorp/go-version
unused github.com/hashicorp/mdns
unused github.com/hashicorp/yamux
unused github.com/klauspost/crc32
unused github.com/kr/logfmt
unused github.com/mitchellh/cli
unused github.com/mitchellh/copystructure
unused github.com/morikuni/aec
unused github.com/mwitkow/go-conntrack
unused github.com/opencontainers/go-digest
unused github.com/opencontainers/image-spec
unused github.com/opencontainers/runc
unused github.com/pkg/profile
unused github.com/posener/complete
unused github.com/prometheus/client_model
unused github.com/prometheus/procfs
unused github.com/smartystreets/gunit
unused github.com/tidwall/assert
unused github.com/tidwall/rtree
unused go.uber.org/atomic
unused go.uber.org/multierr
unused gopkg.in/alecthomas/kingpin.v2
unused gopkg.in/asn1-ber.v1
unused gotest.tools/v3

@nsrip-dd
Copy link
Contributor

nsrip-dd commented Apr 4, 2022

I see. The tidy fails for versions 1.13-1.15 due to a transitive test dependency requiring a Go 1.16 feature (io/fs). From what I can piece together, Go 1.12 works for tidy because it doesn't seem to follow test dependencies outside the main module by default. Or at least, it doesn't care that the test dependencies won't build for Go 1.12. Running with later Go versions doesn't give too different a result. So I'm okay with this change, we'll just need to be careful to be consistent about which Go version we use for running go mod tidy in the future so we don't get surprising changes to go.mod.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug unintended behavior that has to be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

go-chi unknown revision v4.0.0-rc1
5 participants