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

send http responses in header rather than body #90

Merged
merged 4 commits into from
Jun 25, 2024
Merged

Conversation

nickcurie
Copy link
Contributor

@nickcurie nickcurie commented Jun 24, 2024

Notes

  • Moved logging to use opencost's version of logs
  • Moved the deprecated ioutil calls to os / io
  • Removed unnecessary parameters for Turndown Endpoints struct
  • Utilize opencost's protocol package for api responses

Testing

  • Tested endpoints in unit tests

Copy link

@avrodrigues5 avrodrigues5 left a comment

Choose a reason for hiding this comment

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

Based on the code, it looks good that you have moved the error code from the response body to the response JSON.

But based on the original ticket, https://kubecost.atlassian.net/issues/ENG-751?filter=10157, there's a 500 error code. Are you able to repro that and see why that is happening? The ticket says cannot create a Turndown schedule.

Copy link

@nik-kc nik-kc left a comment

Choose a reason for hiding this comment

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

Looks great! One tiny tiny potential change, but otherwise, I approve.

@nik-kc
Copy link

nik-kc commented Jun 25, 2024

Based on the code, it looks good that you have moved the error code from the response body to the response JSON.

But based on the original ticket, https://kubecost.atlassian.net/issues/ENG-751?filter=10157, there's a 500 error code. Are you able to repro that and see why that is happening? The ticket says cannot create a Turndown schedule.

Good catch, Alan, I didn't notice this. I echo this question.

@nickcurie
Copy link
Contributor Author

Yeah if you see my comment here: https://kubecost.atlassian.net/browse/ENG-751?focusedCommentId=124848 the reason why we got the creation failure was due to the k8s api being unresponsive. I recall when I first started working on the ticket we needed to restart the pod because of this. The original issue was not reproducible after that point, so I just proceeded with modernizing parts of this repo since it was written 4 years ago and hasn't really been touched since

@nik-kc
Copy link

nik-kc commented Jun 25, 2024

Yeah if you see my comment here: https://kubecost.atlassian.net/browse/ENG-751?focusedCommentId=124848 the reason why we got the creation failure was due to the k8s api being unresponsive. I recall when I first started working on the ticket we needed to restart the pod because of this. The original issue was not reproducible after that point, so I just proceeded with modernizing parts of this repo since it was written 4 years ago and hasn't really been touched since

I'm happy with that, I think this is good to merge. What say you, @avrodrigues5?

@avrodrigues5
Copy link

Yeah if you see my comment here: https://kubecost.atlassian.net/browse/ENG-751?focusedCommentId=124848 the reason why we got the creation failure was due to the k8s api being unresponsive. I recall when I first started working on the ticket we needed to restart the pod because of this. The original issue was not reproducible after that point, so I just proceeded with modernizing parts of this repo since it was written 4 years ago and hasn't really been touched since

I'm happy with that, I think this is good to merge. What say you, @avrodrigues5?

Yup makes sense to me too. Thanks @nickcurie

@nickcurie nickcurie merged commit 81fbd24 into develop Jun 25, 2024
2 checks passed
@nickcurie nickcurie deleted the nick-eng-751-new branch June 26, 2024 20:12
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.

None yet

3 participants