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

Return original _id in getTotals request #851

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

jiekang
Copy link
Contributor

@jiekang jiekang commented Mar 19, 2024

Fixes #850

Copy link

netlify bot commented Mar 19, 2024

Deploy Preview for eclipsefdn-adoptium-trss ready!

Name Link
🔨 Latest commit 53fcc78
🔍 Latest deploy log https://app.netlify.com/sites/eclipsefdn-adoptium-trss/deploys/65f9d7a24ee8430008c6c633
😎 Deploy Preview https://deploy-preview-851--eclipsefdn-adoptium-trss.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@smlambert smlambert requested a review from llxia March 19, 2024 21:17
@karianna karianna merged commit cb803f2 into adoptium:master Mar 20, 2024
7 checks passed
@llxia
Copy link
Contributor

llxia commented Mar 20, 2024

  • This does not work when URL, buildName, and buildNum are the input parameters. In this case, _id returns null.
  • This is an aggregation API. This change limits us to pass in a set of ids in the future. We cannot aggregate on ids.
  • Return the input in the output is not a standard practice. In general, APIs should strive for clarity, simplicity, and adherence to the principle of "separation of concerns".
  • @karianna I understand it is just one line change, but it has some implications. I would appreciate it if the requested reviewer could have a chance to review before the PR gets merged. This is an API change. The code should be tested before it gets merged.

@smlambert
Copy link
Contributor

@llxia - feel free to revert - we can discuss best plan for metrics gathering in the issue and then re-approach

@llxia
Copy link
Contributor

llxia commented Mar 20, 2024

Thanks @smlambert . I will create a separate PR to remove line 162, preventing any potential confusion down the road.
Sure, let's discuss metrics gathering approaches.

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.

RFE: Return original input _id in getTotals request
4 participants