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

Analytics #261

Merged
merged 7 commits into from
Mar 9, 2021
Merged

Analytics #261

merged 7 commits into from
Mar 9, 2021

Conversation

melchiormoulin
Copy link
Contributor

@melchiormoulin melchiormoulin commented Jan 13, 2021

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.

There are a few changes in here that need to be made because of changes elsewhere in this library since the PR was opened.

In addition I see signs of go fmt not being ran on this code, unhandled errors, and what look like remnants of code copying from other files within the project. There are also some style changes (such as using Id instead of ID).

analytics_test.go Outdated Show resolved Hide resolved
analytics.go Outdated Show resolved Hide resolved
analytics.go Show resolved Hide resolved
analytics.go Outdated Show resolved Hide resolved
analytics.go Outdated Show resolved Hide resolved
analytics_test.go Outdated Show resolved Hide resolved
command/analytics_show.go Outdated Show resolved Hide resolved
command/analytics_show.go Show resolved Hide resolved
command/analytics_show.go Outdated Show resolved Hide resolved
command/analytics_show.go Outdated Show resolved Hide resolved
@theckman theckman added this to the v1.4.0 milestone Feb 25, 2021
@theckman
Copy link
Collaborator

@melchiormoulin I've included this in the v1.4.0 milestone, as we're planning to make v1.5.0 a special release and I want to get as many features / bugfixes as I can into the v1.4.x line.

I don't have a specific timeline for when we hope to get that out, but if you don't feel like you'll have time to dedicate to this PR in the near-term I can target this for v1.6.0.

@melchiormoulin
Copy link
Contributor Author

Hello @theckman It's been a while i've made this pr, i'm not available right now but I can do this this weekend is that ok for you?

@theckman
Copy link
Collaborator

@melchiormoulin most definitely! Thank you.

From team_id or service_id stats on incident.
You can tweak the start and end date and the urgency.
The aggregation is overday for now
The timezone is in UTC for now
@melchiormoulin
Copy link
Contributor Author

You can review it @theckman

@theckman
Copy link
Collaborator

@melchiormoulin thank you so much! I'm currently heads-down on implementing #267 throughout the package, and will take a look at your PR when I finish that (ideally this weekend, or early this coming week at the latest).

analytics.go Outdated Show resolved Hide resolved
@melchiormoulin
Copy link
Contributor Author

Hello @theckman,
Any news ? :)

analytics.go Outdated Show resolved Hide resolved
@theckman
Copy link
Collaborator

theckman commented Mar 8, 2021

@melchiormoulin sorry for the delay in that review, was working on getting the torrent of (still open) PRs raised to add context.Context support to the repo. Missed your comment when I reviewed two days ago.

@melchiormoulin
Copy link
Contributor Author

Hello @theckman no worries, this is done.

@theckman
Copy link
Collaborator

theckman commented Mar 9, 2021

Thank you so much for the contribution. I'll merge it shortly, and it will be included eventually in the v1.4.0 release. We may still be a few days / weeks out from that release being cut.

@theckman theckman merged commit 3366469 into PagerDuty:master Mar 9, 2021
@melchiormoulin
Copy link
Contributor Author

Hello @theckman,
Do you know when the release will happen please ?

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