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 support for '/_fields' endpoint #104

Merged
merged 5 commits into from
Mar 29, 2022
Merged

Add support for '/_fields' endpoint #104

merged 5 commits into from
Mar 29, 2022

Conversation

lukasmalkmus
Copy link
Collaborator

@lukasmalkmus lukasmalkmus commented Feb 22, 2022

This has long been outstanding but went unnoticed: There has been a new /_info endpoint for a while, now. This is basically a list of dataset infos as returned by a normal call to :id/info. The /_stats call shares the same response model, apart from omitting the fields so this was never an issue. But it is desirable to have the separate call available.

I was able to refactor this slightly to compensate for the missing fields and enhanced the documentation along the way.

As explained by @mhr3, the /_info endpoint is deprecated and - after discussion - will be replaced by /_fields. The rest of the information that could be obtained through /_info is still available through /_stats. The recent commit accounts for that change but I kept the improvements to the DatasetInfo and DatasetStats types as well as the documentation improvements. Until the change is live, this PR will remain in draft status.

After more evaluation, there will be one new endpoint introduced with this PR: /_fields, for retrieving fields of all datasets of a deployment. However, in addition, /_info is deleted as it is essentially the infomration provided by /_fields plus some information only useful for the frontend but not actual costumers. The recent commit accounts for that change but I kept the improvements to the DatasetInfo and DatasetStats types as well as the documentation improvements. Until the change is live, this PR will remain in draft status.

Needs #106.

@lukasmalkmus lukasmalkmus added the enhancement New feature or request label Feb 22, 2022
@lukasmalkmus lukasmalkmus added this to the v0.10.0 milestone Feb 22, 2022
@lukasmalkmus lukasmalkmus enabled auto-merge (rebase) February 22, 2022 18:39
@mhr3
Copy link

mhr3 commented Feb 22, 2022

We'll drop the /_infos endpoint, so let's not add it here

@lukasmalkmus lukasmalkmus marked this pull request as draft February 23, 2022 10:12
auto-merge was automatically disabled February 23, 2022 10:12

Pull request was converted to draft

@lukasmalkmus lukasmalkmus changed the title Add Infos call Add support for '/_fields' endpoint Feb 24, 2022
@codecov
Copy link

codecov bot commented Feb 25, 2022

Codecov Report

Merging #104 (6e67a49) into main (e77ec89) will increase coverage by 0.06%.
The diff coverage is 81.81%.

❗ Current head 6e67a49 differs from pull request most recent head 40863e8. Consider uploading reports for the commit 40863e8 to get more accurate results

@@            Coverage Diff             @@
##             main     #104      +/-   ##
==========================================
+ Coverage   71.85%   71.91%   +0.06%     
==========================================
  Files          41       41              
  Lines        1304     1314      +10     
==========================================
+ Hits          937      945       +8     
- Misses        222      223       +1     
- Partials      145      146       +1     
Impacted Files Coverage Δ
axiom/apl/format.go 100.00% <ø> (ø)
axiom/monitors.go 68.62% <ø> (ø)
axiom/notifiers.go 64.10% <ø> (ø)
axiom/orgs.go 66.19% <ø> (ø)
axiom/query/aggregation.go 84.21% <ø> (ø)
axiom/query/filter.go 88.00% <ø> (ø)
axiom/query/kind.go 70.00% <ø> (ø)
axiom/query/result.go 72.00% <ø> (ø)
axiom/starred.go 58.33% <ø> (ø)
axiom/tokens.go 76.66% <ø> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e4f1c7...40863e8. Read the comment docs.

@lukasmalkmus lukasmalkmus marked this pull request as ready for review February 25, 2022 12:26
@lukasmalkmus
Copy link
Collaborator Author

This is ready for review and can be merged when CI passes on staging.

@lukasmalkmus lukasmalkmus enabled auto-merge (rebase) February 25, 2022 12:26
@lukasmalkmus lukasmalkmus force-pushed the add-infos-call branch 3 times, most recently from 46c467e to 51e6ab6 Compare March 8, 2022 13:27
@lukasmalkmus
Copy link
Collaborator Author

Updated to PR description to reflect the latest state of the discussion.

I re-ran tests and made some minor adjustments.

@github-actions
Copy link

This PR/issue depends on:

@lukasmalkmus
Copy link
Collaborator Author

This is finally ready to merge!

Copy link
Member

@bahlo bahlo left a comment

Choose a reason for hiding this comment

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

LGTM

@lukasmalkmus lukasmalkmus merged commit 9aef938 into main Mar 29, 2022
@lukasmalkmus lukasmalkmus deleted the add-infos-call branch March 29, 2022 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants