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.

Sign up for GitHub

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

Jump to bottom

Moving outputs/transport => common/transport #16734

Merged
merged 9 commits into from
Mar 4, 2020
Merged

Moving outputs/transport => common/transport #16734

merged 9 commits into from
Mar 4, 2020

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Mar 2, 2020 •
edited by urso
Loading

What does this PR do?

  • Moves the transport package from libbeat/outputs/transport to libbeat/common/transport.
  • Removes outputs/tls.go and updates all it's usages.
  • Replaces *Config parameters (pass-by-pointer) with Config parameters (pass-by-value) in functions/methods wherever safe.

Why is it important?

The code in this package is used in places other than outputs.

Developer Docs

This PR introduces breaking changes to the API provided by libbeat.

  • If you were importing libbeat/outputs/transport, in most cases you must now import libbeat/common/transport instead.
  • If you were importing libbeat/outputs/transport or libbeat/outputs for using any TLS-related symbols, you must now import libbeat/common/transport/tlscommon instead.
  • If you were calling libbeat/outputs/transport.MakeDialer with a *Config object as the first argument, you must now call libbeat/common/transport.MakeDialer with a Config object as the first argument instead.
  • If you were calling libbeat/outputs/transport.NewClient with a *Config object as the first argument, you must now call libbeat/common/transport.NewClient with a Config object as the first argument instead.
  • If you were calling libbeat/outputs/transport.NewClientWithDialer with a *Config object as the second argument, you must now call libbeat/common/transport.NewClientWithDialer with a Config object as the second argument instead.
  • If you were calling libbeat/outputs/transport.Dial with a *Config object as the first argument, you must now call libbeat/common/transport.Dial with a Config object as the first argument instead.

Checklist

  • My code follows the style guidelines of this project
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files
  • [ ] I have added tests that prove my fix is effective or that my feature works

Related issues

@ycombinator ycombinator requested a review from urso March 2, 2020 19:58
@ycombinator ycombinator requested a review from a team as a code owner March 2, 2020 19:58
@ycombinator ycombinator force-pushed the lb-refactor-transport branch 2 times, most recently from 27b366f to 2ea71ee Compare March 2, 2020 21:08
@ycombinator ycombinator mentioned this pull request Mar 2, 2020
Merged
2 tasks
@ycombinator ycombinator force-pushed the lb-refactor-transport branch from 2ea71ee to 67c6fbb Compare March 3, 2020 00:25
@urso
Copy link

urso commented Mar 3, 2020

Looks like we need to rebase.

This is an API breaking change, as a many things got moved that other code (also community code) might rely upon. Please add a developers changelog. Would be nice to include some 'Developer Change' section in the PR description giving some instructions on what needs to be done to adapt existing code to the changes.

ping @elastic/apm-server

@simitt simitt mentioned this pull request Mar 3, 2020
Merged
3 tasks
@ycombinator ycombinator force-pushed the lb-refactor-transport branch from 67c6fbb to 6c1831a Compare March 3, 2020 15:29
@ycombinator ycombinator mentioned this pull request Mar 3, 2020
Closed
@ycombinator
Copy link
Contributor Author

Looks like we need to rebase.

Done!

This is an API breaking change, as a many things got moved that other code (also community code) might rely upon. Please add a developers changelog.

Added in b80a8f5cc3714fd60ee59444c536c4a33a54366b.

Would be nice to include some 'Developer Change' section in the PR description giving some instructions on what needs to be done to adapt existing code to the changes.

Done, PR description updated.

ping @elastic/apm-server

I created elastic/apm-server#3416 as well.

@ycombinator ycombinator force-pushed the lb-refactor-transport branch from 585ac4a to b679601 Compare March 3, 2020 18:11
@ycombinator
Copy link
Contributor Author

@urso This is ready for re-review, when you get a chance. Thanks!

@ycombinator ycombinator force-pushed the lb-refactor-transport branch from 9f7a152 to f3df370 Compare March 4, 2020 00:15
@urso urso added the release-note:dev_docs Contains a Dev Docs section label Mar 4, 2020
urso
urso approved these changes Mar 4, 2020
View reviewed changes
Copy link

@urso urso left a comment

Choose a reason for hiding this comment

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

LGTM

I like the line count changes :)

@ycombinator
Copy link
Contributor Author

@urso Just want to clarify something — even though this is a developer breaking change, we want to backport this to 7.x, correct? We'll need it for the larger ES output refactoring PR.

@urso
Copy link

urso commented Mar 4, 2020

Just want to clarify something — even though this is a developer breaking change, we want to backport this to 7.x, correct? We'll need it for the larger ES output refactoring PR.

TBH, tough call :)
But if we want to push forward with the output changes, in order to make the output properly reloadable it is a must to backport.

One option for 7.x is to add 'compatbility' with existing code by introducing aliases (this use case is exactly why aliases have been introduced in go in the first place).

@ycombinator
Copy link
Contributor Author

Okay, sounds good. I will setup a backport PR (once this PR is merged) but amend it to include aliases from the old => new exported symbols.

@ycombinator ycombinator force-pushed the lb-refactor-transport branch from f3df370 to 7cd61b7 Compare March 4, 2020 15:11
@ycombinator
Copy link
Contributor Author

Jenkins CI failures are unrelated (they are also seen on master builds) and Travis CI is green. Merging.

@ycombinator ycombinator merged commit 6352ea1 into elastic:master Mar 4, 2020
@ycombinator ycombinator deleted the lb-refactor-transport branch March 4, 2020 18:09
@zube zube bot added [zube]: Done and removed [zube]: Inbox labels Mar 4, 2020
@ycombinator ycombinator mentioned this pull request Mar 4, 2020
Merged
ycombinator added a commit that referenced this pull request Mar 5, 2020
@ycombinator
[7.x] Moving outputs/transport => common/transport (#16734) (#16820)
0b6e440 
* Moving outputs/transport => common/transport (#16734)

* Moving outputs/transport => common/transport

* Removing outputs/tls.go

* Pass config object by value (not pointer)

* Fixing formatting of imports

* Adding developer CHANGELOG entries

* Fixing formatting of imports

* Running go mod tidy

* Moving MakeDialer into transport

* Running go mod tidy

* Remove breaking changes entry from developer CHANGELOG

* Bring back old packages for backwards compat

* Adding back libbeat/outputs/tls.go

* Fixing up libbeat/outputs/transport/tls.go

* Restoring MakeDialer to original place

* Missed an arg

* Restoring *Config params to old code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@urso urso urso approved these changes

Assignees
No one assigned
Labels
libbeat refactoring release-note:dev_docs Contains a Dev Docs section Team:Integrations Label for the Integrations team v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove libbeat/outputs/transport
3 participants
@ycombinator @urso @andresrc