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

Add slack connection apis #432

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

GiedriusS
Copy link
Contributor

@GiedriusS GiedriusS commented Mar 16, 2022

Add code for working with the relatively new Slack connection APIs as defined here: https://developer.pagerduty.com/api-reference/b3A6MTEyMDk1NDU-list-slack-connections and in other pages.

Tests pass:

/bin/go test -timeout 30s -run ^(TestSlackConnection_Delete|TestSlackConnection_List|TestSlackConnection_Update|TestSlackConnection_Get)$ github.com/PagerDuty/go-pagerduty

ok  	github.com/PagerDuty/go-pagerduty	0.005s

Copy link
Collaborator

@theckman theckman left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. I think there are a few changes to be made before we can merge it.

Also, can you please squash the commits of the branch so all of these changes are within a single commit? It helps keep the history of the primary branch more clear.


// SlackConnection is an entity that represents a Slack connections as per the
// documentation https://developer.pagerduty.com/api-reference/c2NoOjExMjA5MzMy-slack-connection.
type SlackConnection struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This type seems to be missing the ID string field.

Comment on lines +34 to +37
type SlackConnectionObject struct {
SlackConnection
APIObject
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We intend to no longer use embedding in this project. Please list the fields on the type explicitly. See #318 for more details.

That said, I think we should probably just elide this type entirely and put all of the fields in the SlackConnection type.

// ListSlackConnectionsResponse is an API object returned by the list function.
type ListSlackConnectionsResponse struct {
Connections []SlackConnectionObject `json:"slack_connections"`
APIListObject
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment around embedding as well.

}

// CreateSlackConnectionWithContext creates a Slack connection.
func (c *Client) CreateSlackConnectionWithContext(ctx context.Context, slackTeamID string, s SlackConnection) (SlackConnectionObject, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove WithContext from the name, since there is not a non-context version.

}

// GetSlackConnection gets a Slack connection.
func (c *Client) GetSlackConnectionWithContext(ctx context.Context, slackTeamID, connectionID string) (SlackConnectionObject, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove WithContext from the name, since there is not a non-context version.

}

// DeleteSlackConnectionWithContext deletes a Slack connection.
func (c *Client) DeleteSlackConnectionWithContext(ctx context.Context, slackTeamID, connectionID string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove WithContext from the name, since there is not a non-context version.

}

// UpdateSlackConnectionWithContext updates an existing Slack connection.
func (c *Client) UpdateSlackConnectionWithContext(ctx context.Context, slackTeamID, connectionID string, s SlackConnection) (SlackConnectionObject, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove WithContext from the name, since there is not a non-context version.

}

// ListSlackConnectionsWithContext lists Slack connections.
func (c *Client) ListSlackConnectionsWithContext(ctx context.Context, slackTeamID string, o ListSlackConnectionsOptions) (*ListSlackConnectionsResponse, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove WithContext from the name, since there is not a non-context version. Also, I don't think this should return a pointer type. We're going to be moving away from those when it makes sense (#270).

Comment on lines 501 to +520
func dupeRequest(r *http.Request) (*http.Request, error) {
data, err := ioutil.ReadAll(r.Body)
if err != nil {
return nil, fmt.Errorf("failed to copy request body: %w", err)
}
var data []byte
// Body can be nil in GET requests, for example.
if r.Body != nil {
bodyData, err := ioutil.ReadAll(r.Body)
if err != nil {
return nil, fmt.Errorf("failed to copy request body: %w", err)
}

_ = r.Body.Close()
_ = r.Body.Close()

data = bodyData
}

dreq := r.Clone(r.Context())

r.Body = ioutil.NopCloser(bytes.NewReader(data))
dreq.Body = ioutil.NopCloser(bytes.NewReader(data))
if data != nil {
r.Body = ioutil.NopCloser(bytes.NewReader(data))
dreq.Body = ioutil.NopCloser(bytes.NewReader(data))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please raise this chunk as an isolated PR?

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.

2 participants