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

Go 1.17: Stop using ; as a separator in URL query strings #8143

Merged
merged 2 commits into from Sep 7, 2021

Conversation

zmb3
Copy link
Collaborator

@zmb3 zmb3 commented Sep 3, 2021

Go 1.17 (by default) disallows using ; as a separator, which causes our tests to fail on Go 1.17.

Go 1.17 also includes an escape hatch to revert to the legacy behavior, but it's a new API and would prevent our code from compiling on 1.16. Since we only do this in test code, it's easiest just to switch to & so we don't have a breakage when we start compiling with 1.17.

See also: golang/go#25192

Error:

2021/09/03 10:21:57 http: URL query contains semicolon, which is no longer a supported separator; parts of the query may be stripped when parsed; see golang.org/issue/25192

----------------------------------------------------------------------
FAIL: apiserver_test.go:473: WebSuite.TestSAMLSuccess

apiserver_test.go:524:
    c.Assert(u.Scheme+"://"+u.Host+u.Path, Equals, fixtures.SAMLOktaSSO)
... obtained string = ":///web/msg/error/login"
... expected string = "https://dev-813354.oktapreview.com/app/gravitationaldev813354_teleportsaml_1/exkafftca6RqPVgyZ0h7/sso/saml"

Go 1.17 (by default) disallows using ; as a separator. Our tests fail
on Go 1.17 with the following error. Since we only do this in test
code, it's easiest just to switch to & so we don't have a breakage
when we start compiling with 1.17.

See also: golang/go#25192

Error:

    2021/09/03 10:21:57 http: URL query contains semicolon, which is no longer a supported separator; parts of the query may be stripped when parsed; see golang.org/issue/25192

    ----------------------------------------------------------------------
    FAIL: apiserver_test.go:473: WebSuite.TestSAMLSuccess

    apiserver_test.go:524:
        c.Assert(u.Scheme+"://"+u.Host+u.Path, Equals, fixtures.SAMLOktaSSO)
    ... obtained string = ":///web/msg/error/login"
    ... expected string = "https://dev-813354.oktapreview.com/app/gravitationaldev813354_teleportsaml_1/exkafftca6RqPVgyZ0h7/sso/saml"
Copy link
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for the fix - I noticed this one too, but you beat me to it as usual.

Copy link
Contributor

@russjones russjones left a comment

Choose a reason for hiding this comment

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

Bot.

@zmb3 zmb3 enabled auto-merge (squash) September 5, 2021 14:56
@zmb3 zmb3 merged commit 992c10f into master Sep 7, 2021
@zmb3 zmb3 deleted the zmb3/go-117-url-fix branch September 7, 2021 02:56
zmb3 added a commit that referenced this pull request Sep 23, 2021
Go 1.17 (by default) disallows using ; as a separator. Our tests fail
on Go 1.17 with the following error. Since we only do this in test
code, it's easiest just to switch to & so we don't have a breakage
when we start compiling with 1.17.

See also: golang/go#25192

Error:

    2021/09/03 10:21:57 http: URL query contains semicolon, which is no longer a supported separator; parts of the query may be stripped when parsed; see golang.org/issue/25192

    ----------------------------------------------------------------------
    FAIL: apiserver_test.go:473: WebSuite.TestSAMLSuccess

    apiserver_test.go:524:
        c.Assert(u.Scheme+"://"+u.Host+u.Path, Equals, fixtures.SAMLOktaSSO)
    ... obtained string = ":///web/msg/error/login"
    ... expected string = "https://dev-813354.oktapreview.com/app/gravitationaldev813354_teleportsaml_1/exkafftca6RqPVgyZ0h7/sso/saml"
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.

None yet

3 participants