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

Remove gorilla/context #81

Merged
merged 8 commits into from
Nov 23, 2020
Merged

Remove gorilla/context #81

merged 8 commits into from
Nov 23, 2020

Conversation

bradwest-wk
Copy link
Contributor

@bradwest-wk bradwest-wk commented Nov 9, 2020

Goal

The goal of this PR is to remove dependence on gorilla/context, which is deprecated. Usages are replaced with the stdlib context package, specifically with calls to net/http's Request.Context() and context.WithValue().

In addition to this update, I've updated to go 1.15 and go modules and version bumped to v2. The commit history should be straight forward. In particular, 4ca0815 shows the changes I made to v1.

Test

The interface remained the same. Consumers of go-rest should not use gorilla/context. Should be tested against services using this library (analytics_gateway).

@aviary2-wf
Copy link

aviary2-wf commented Nov 9, 2020

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

@bradwest-wk bradwest-wk force-pushed the remove-gorilla-context branch 2 times, most recently from 22fdcca to 1abdb25 Compare November 9, 2020 22:54
@bradwest-wk bradwest-wk force-pushed the remove-gorilla-context branch 2 times, most recently from 51f1577 to cb8fedc Compare November 10, 2020 21:01
rest/context_test.go Outdated Show resolved Hide resolved
tomdeering-wf
tomdeering-wf previously approved these changes Nov 13, 2020
Copy link
Contributor

@tomdeering-wf tomdeering-wf left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM. As you say, it doesn't look like you've made any changes that would break us.

rest/context.go Outdated Show resolved Hide resolved
rest/context.go Outdated Show resolved Hide resolved
rest/context.go Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@trentonsmith-wf
Copy link
Contributor

security +1

  • Aviary-triggering code was just copied from v1, no changes made.

@trentonsmith-wf
Copy link
Contributor

security +1
Same triggered keywords are just in renamed files.

https://golang.org/ref/mod#major-version-suffixes

Major version suffixes are not allowed at major versions v0 or v1. There is
no need to change the module path between v0 and v1 because v0 versions are
unstable and have no compatibility guarantee. Additionally, for most modules,
v1 is backwards compatible with the last v0 version; a v1 version acts as a
commitment to compatibility, rather than an indication of incompatible
changes compared with v0.
@bradwest-wk
Copy link
Contributor Author

QA +1

Pushed to test branch and tested against the analytics_gateway. No issues handling contextual information.

@bradwest-wk
Copy link
Contributor Author

@trentonsmith-wf @blakemorris-wk I apologize for all the false starts here. I misunderstood the limitations of major version suffixes and (earlier) stupidly assumed this lib was at a stable release. Should be good to go now.

@bradwest-wk
Copy link
Contributor Author

QA +1 refresh @Workiva/release-management

@bradwest-wk bradwest-wk merged commit 9af7759 into master Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants