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

Added initial insights #565

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

Added initial insights #565

wants to merge 69 commits into from

Conversation

Fank
Copy link
Contributor

@Fank Fank commented Sep 12, 2022

What type of PR is this?

feature

What this PR does / why we need it:

Adds cloud app insight: https://developer.atlassian.com/cloud/insight/rest/

Which issue(s) this PR fixes:

Relates to #495

Special notes for your reviewer:

Missing ToDos:

  • Added documentation (will be added later)
  • Added tests

Additional documentation e.g., usage docs, etc.:

@andygrunwald
Copy link
Owner

Thanks for this Pull Request. Overall it looks quite good.
I would like to run through a few points with you

model package

What is the thought behind the own model package?
The package contains all structs in the top-level package right now.

Are there name conflicts?

Insight API vs. single services

In the current implementation (for object Get objecttype {id} attributes), you call it like

client.Insight.GetObjectTypeAttributes(...)

What do you think about an API like

client.Insight.Objecttype.GetAttributes(...)

The sublevel (here Objecttype) would be in line with the documentation navigation structure at https://developer.atlassian.com/cloud/insight/rest/api-group-objecttype/#api-objecttype-id-attributes-get

Unit tests

It would be nice to have a few (you listed those as open TODOs already)

Checking of status codes

The code

switch res.StatusCode {
case http.StatusUnauthorized:
	return nil, fmt.Errorf("%s: %w", req.URL.String(), ErrUnauthorized)
case http.StatusNotFound:
	return nil, fmt.Errorf("%s: %w", req.URL.String(), ErrNotFound)
case http.StatusInternalServerError:
	return nil, fmt.Errorf("%s: %w", req.URL.String(), ErrUnknown)
}

It looks like it is nearly all the same.
Would we be able to make this more generic and place this in the Do method (where the request is executed)?

@Fank
Copy link
Contributor Author

Fank commented Sep 13, 2022

model package

What is the thought behind the own model package? The package contains all structs in the top-level package right now.

Are there name conflicts?

No there are not (yet), buts its an app from the marketplace for the cloud service and because of that i would like to separate it because its tracks a completely different version and naming.
So it easier to find out where are those structs are coming from and why are they there.

Insight API vs. single services

In the current implementation (for object Get objecttype {id} attributes), you call it like

client.Insight.GetObjectTypeAttributes(...)

What do you think about an API like

client.Insight.Objecttype.GetAttributes(...)

The sublevel (here Objecttype) would be in line with the documentation navigation structure at https://developer.atlassian.com/cloud/insight/rest/api-group-objecttype/#api-objecttype-id-attributes-get

Yes i totally missed that, its changed as written above.

Checking of status codes

The code

switch res.StatusCode {
case http.StatusUnauthorized:
	return nil, fmt.Errorf("%s: %w", req.URL.String(), ErrUnauthorized)
case http.StatusNotFound:
	return nil, fmt.Errorf("%s: %w", req.URL.String(), ErrNotFound)
case http.StatusInternalServerError:
	return nil, fmt.Errorf("%s: %w", req.URL.String(), ErrUnknown)
}

It looks like it is nearly all the same. Would we be able to make this more generic and place this in the Do method (where the request is executed)?

This is a proof of concept to check if this is a solid way to handle errors, so far it looks good.
I would like to replace the existing checks for bad status code with this behavior in general, because i see the same status codes in Jira REST and Service Desk too.
Is this fine to replace the current status code handling with this attempt?
If yes i would like to separate it into a new PR

@Fank Fank mentioned this pull request Sep 15, 2022
36 tasks
@andygrunwald
Copy link
Owner

Thanks. This PR is still on my list to review and move forward. It might take a few days.

@github-actions github-actions bot added the conflicts Indicates merge conflicts label Mar 12, 2023
@ChristianKniep
Copy link

ChristianKniep commented Jun 14, 2023

Hey there, what is the status of the asset integration. I am going to need that soon(-ish), so happy to help.
EDIT: Commenting before reading the complete information... there are other PRs already which are newer. :)

@Fank
Copy link
Contributor Author

Fank commented Jun 14, 2023

@ChristianKniep this PR is abandoned
Currently working on that here ctreminiom/go-atlassian#204

@Fank Fank closed this Jun 14, 2023
@andygrunwald andygrunwald reopened this Jun 25, 2023
@andygrunwald
Copy link
Owner

Just Reopening this.
I think there are a few good ideas in this PR.
Right now, my time is pretty limited, still, I plan to come back to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflicts Indicates merge conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants