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

BCDA-7416: Remove duplicate logging #148

Merged
merged 13 commits into from
Dec 4, 2023
Merged

BCDA-7416: Remove duplicate logging #148

merged 13 commits into from
Dec 4, 2023

Conversation

karinamzalez
Copy link
Contributor

@karinamzalez karinamzalez commented Nov 22, 2023

🎫 Ticket

https://jira.cms.gov/browse/BCDA-7416

🛠 Changes

(What was added, updated, or removed in this PR.)

  • removes duplicate logging in api.go
  • adds comments for future refactors that would improve logging

ℹ️ Context for reviewers

(Background context, more in-depth details of the implementation, and anything else you'd like to call out or ask reviewers.)

  • in service/main/main.go > listIPs() : replace panic with ssas.Logger.Fatal
  • in server.go > NewServer > ln 164: add return statement after error is logged, so more generic log outside of this condition doesn't log error again.
  • in public/api.go: refactor error logging in ValidateSecret.
  • Adds three "TODOs" to bring attention to potential areas for refactor to streamline logging. TODOs are for the following:
    • Remove logging from JSONError and assert that 130 use cases of this function still have, more specific, logging outside of function
    • Remove OperationFailed and refactor use cases to use ssas.Logger
    • Refactor ChooseSigningKey so error logging happens outside of function

✅ Acceptance Validation

(How were the changes verified? Did you fully test the acceptance criteria in the ticket? Provide reproducible testing instructions and screenshots if applicable.)

🔒 Security Implications

  • This PR adds a new software dependency or dependencies.
  • This PR modifies or invalidates one or more of our security controls.
  • This PR stores or transmits data that was not stored or transmitted before.
  • This PR requires additional review of its security implications for other reasons.

If any security implications apply, add Jason Ashbaugh (GitHub username: StewGoin) as a reviewer and do not merge this PR without his approval.

ssas/service/server.go Outdated Show resolved Hide resolved
revert spacing in swagger meta
re-add new lines in main.go swagger meta
@laurenkrugen-navapbc
Copy link
Contributor

This looks good overall, just need a couple things:

  1. looks like tests are failing. are the unit-tests updated to reflect the changes?
  2. can we also please update the PR title to be in the format of BCDA-XXXX: <...>?

@karinamzalez karinamzalez changed the title Karina/Remove duplicate logging BCDA-7416: Remove duplicate logging Nov 30, 2023
Copy link
Contributor

@laurenkrugen-navapbc laurenkrugen-navapbc left a comment

Choose a reason for hiding this comment

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

looks good, thanks for updating! 😃

@karinamzalez karinamzalez merged commit bde4582 into master Dec 4, 2023
3 checks passed
@karinamzalez karinamzalez deleted the karina/bcda-7416 branch December 4, 2023 15:27
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