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

Adding the v2/Teams API #6

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

Adding the v2/Teams API #6

wants to merge 10 commits into from

Conversation

vandr0iy
Copy link

@vandr0iy vandr0iy commented Dec 6, 2023

Hi! I've noted that in this repo there's no API for v2/Teams, so I added all the GET calls - and also some of the other ones. Please, be kind: this is my literal first OSS contribution in typescript 😅

@danopia
Copy link
Member

danopia commented Dec 6, 2023

Hello, thanks for the work. I am on vacation currently but can check in for a review in the next week. I assume you've already used the new APIs and they work for your own needs?

@vandr0iy
Copy link
Author

Hi @danopia! basically, I was writing a script that's using the Teams and the Users API - however, when I found this package I only found the latter being implemented - so I added the former.

While I wrote the script I was able to successfully invoke all of the Teams API calls at least once (+ that one v2/users call that I added myself). Does that answer your question?

Copy link
Member

@danopia danopia left a comment

Choose a reason for hiding this comment

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

I have two specific requests on this

// Common API client contract
interface ApiClient {
fetchJson(opts: {
method: "GET" | "POST" | "DELETE";
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is the first DELETE call in this project, and so the new method needs to be also added to client.ts:

method?: 'GET' | 'POST';

@@ -10,10 +10,37 @@ export interface ResultPage<T> {
data: Array<T>;
}

export interface Included {
export interface TeamsResultPage<T> {
Copy link
Member

Choose a reason for hiding this comment

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

Did datadog really need to add another pagination structure within v2 😅 LGTM

Comment on lines +102 to +104
async createTeam(name: string): Promise<string> {
const words = [...name.toLowerCase().matchAll(/[a-z0-9]+/g)].map((x) => x[0])
const handle = words.join("-")
Copy link
Member

Choose a reason for hiding this comment

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

This is slightly magic. Does this string manipulation follow a particular recommendation from datadog?
Continued:

Copy link
Author

@vandr0iy vandr0iy Dec 22, 2023

Choose a reason for hiding this comment

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

oof. I'm afraid I leaked out some business logic from my script here 😅 refactoring in progress

Comment on lines +111 to +114
attributes: {
name: name,
handle: handle
},
Copy link
Member

Choose a reason for hiding this comment

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

I see that there are additional possible attributes for this API, so perhaps we can accept a second optional parameter extraAttributes that lets the caller override handle and/or pass more attributes for the team.

Suggested change
attributes: {
name: name,
handle: handle
},
attributes: {
name: name,
handle: handle,
...extraAttributes,
},

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