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

Refactor to work correctly with V2 API #23

Merged
merged 10 commits into from Sep 4, 2016
Merged

Refactor to work correctly with V2 API #23

merged 10 commits into from Sep 4, 2016

Conversation

dthagard
Copy link
Contributor

@dthagard dthagard commented Sep 3, 2016

  • Overhauled the SDK to work correctly with the PagerDuty v2 API.
  • Eliminated all golint warnings including adding documentation.
  • Hid some methods on Client that should not be exposed.
  • Changed behavior on create/update of objects to return the resultant object.
  • Fixed some struct definitions so that they are serialized to JSON correctly.
  • Implemented some missing methods.
  • Vendored external required libraries.

dthagard and others added 8 commits August 28, 2016 17:52
@dthagard
Revamped the PagerDuty API.
f68cb75 
* Added documentation to all exported types.
* Hid some internal functions to client.
* Updated Team methods to return Team interface on get/set operations.
* Fixed JSON conversion for Team object.
* Removed unused fields from Integration object.
* Updated field names to match official documentation.
* Fixed JSON conversion for OnCall object.
* implemented UpdateEscalationPolicy.
* Vendored third-party libraries.
@dthagard
Updated service.CreateService to return a Service interface.
e6f0142
@dthagard
Fixed typo in error message.
f8aa430
@dthagard
Fixed typo in error message.
bc72d76
Updated functionality for client, escalation_policy, schedule, and se…
e1d2f16 
…rvice.

* Added ability to parse and return the error object from PagerDuty API.
* Added missing RepeatEnabled to escalation_policy.
* Added return of Schedule for create/update.
* Added return of Service for create/update.
Enhanced error response when creating,updating,getting resources.
755d74a
Fixed problem with error response handling.
7d106b7
@dthagard
Updated MaintenanceWindow and User to return objects on create/update.
e1c887d
@dthagard dthagard mentioned this pull request Sep 3, 2016
Closed
@Luzifer
Copy link

Luzifer commented Sep 3, 2016 •
edited
Loading

Vendored external required libraries.

Please don't do this. This will cause trouble when vendoring your library into a binary project.

In Go, dependency management is a concern of the binary author. Libraries with vendored dependencies are very difficult to use; so difficult that it is probably better said that they are impossible to use. There are many corner cases and edge conditions that have played out in the months since vendoring was officially introduced in 1.5. (You can dig in to one of these forum posts if you’re particularly interested in the details.) Without getting too deep in the weeds, the lesson is clear: libraries should never vendor dependencies.
(Source: Go best practices, six years in)

As an example: When my project uses (and vendors) aws/aws-sdk-go and also a library I'm using uses that SDK, we both vendor different versions this will just blow up as there are conflicting versions.

Another example: if I'm using logrus (I saw you've vendored that one) Go has to decide whether to use the version in vendor/github.com/Sirupsen/logrus OR the version in vendor/github.com/PagerDuty/go-pagerduty/vendor/github.com/Sirupsen/logrus…

In the end: this will cause trouble…

@ranjib
Copy link
Contributor

ranjib commented Sep 3, 2016

Awesome. Dan, thanks for all the hard work. As Knut mentioned, can you
remove the vendor bits. I am planning to nuke logrus dependency. I want to
keep the library with less dependencies (I can only see go-querystring
being required as of now )

On Sep 3, 2016 12:38 PM, "Knut Ahlers" [email protected] wrote:

Vendored external required libraries.

Please don't do this. This will cause trouble when vendoring your library
into a binary project.

In Go, dependency management is a concern of the binary author. Libraries
with vendored dependencies are very difficult to use; so difficult that it
is probably better said that they are impossible to use. There are many
corner cases and edge conditions that have played out in the months since
vendoring was officially introduced in 1.5. (You can dig in to one of these
forum posts if you’re particularly interested in the details.) Without
getting too deep in the weeds, the lesson is clear: libraries should never
vendor dependencies.
(Source: Go best practices, six years in
https://peter.bourgon.org/go-best-practices-2016/#dependency-management)

As an example: When my project uses (and vendors) aws/aws-sdk-go and also
a library I'm using uses that SDK, we both vendor different versions this
will just blow up as there are conflicting versions. Also for example if
I'm using logrus (I saw you've vendored that one) Go has to decide whether
to use the version in vendor/github.com/Sirupsen/logrus OR the version in
vendor/github.com/PagerDuty/go-pagerduty/vendor/github.com/Sirupsen/logrus
…

In the end: this will cause trouble…

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#23 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AATTkv0fNsPUezp8Fz4-BoplytgJDbi3ks5qmcybgaJpZM4J0XzF
.

@Luzifer
Copy link

Luzifer commented Sep 3, 2016

Yep definitely! Thank you very much for rewriting this. As far as I've seen the code this looks very good to me. 👍

@ranjib
Copy link
Contributor

ranjib commented Sep 4, 2016

🏆

@ranjib ranjib merged commit a1989ee into PagerDuty:master Sep 4, 2016
@dthagard dthagard deleted the refactor_to_v2 branch October 24, 2016 12:40
mLupine pushed a commit to mLupine/go-pagerduty that referenced this pull request May 27, 2020
@heimweh
Bump Go version (PagerDuty#23)
9fcc6eb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dthagard @Luzifer @ranjib