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-7257: Split opt out utilities into a separate module #861

Merged
merged 20 commits into from
Jul 24, 2023

Conversation

kyeah
Copy link
Contributor

@kyeah kyeah commented Jul 17, 2023

🎫 Ticket

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

🛠 Changes

Create a new Golang module for optout code that can be imported separate from BCDA code. This includes:

  • All related models (SuppressionFile --> OptOutFile, Suppression --> OptOutRecord, SuppressionFileMetadata --> OptOutFilenameMetadata)
  • Utility function for parsing metadata from filenames
  • Utility function for parsing a single line from an opt out file

All other changes to files are just updating the model names or changing the model fields from private to public (e.g. suppression.fileID --> suppression.FileID) since the models are now in a separate module.

For reviewers, I would recommend skimming the existing files and focusing mainly on the new optout folder 🙂

ℹ️ Context for reviewers

AB2D and DPC are currently in the process of automating data-sharing opt-out requests. It is expected that the file formats will be consistent with the existing files that BCDA receives, so a lot of the existing Go code is reusable.

DPC is likely going to follow the pre-established pattern of building lambdas in Go, so we expect that BCDA's code could be directly used as part of the lambda. This PR pulls out generic utilities that are not related to BCDA's implementation in any way (e.g. reading from the local filesystem or saving to the BCDA database.)

✅ Acceptance Validation

  • Unit tests continue to pass
  • Was able to create a new local go program and import these utilities using go get github.com/CMSgov/bcda-app/output@777710bbe26fba867d10a61a84f68a1b1fdb1f76.

🔒 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.

@kyeah kyeah marked this pull request as ready for review July 18, 2023 16:13
go.work Outdated
Copy link
Contributor

@laurenkrugen-navapbc laurenkrugen-navapbc Jul 19, 2023

Choose a reason for hiding this comment

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

EDIT: you can disregard my comment, it's not applicable here!

this is neat. do we have to make changes in both modules or does one pack up the changes and apply it to the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the workspace file basically just tells each module about the other, so this makes the optout module visible to the main bcda module.

When bcda is packaged, it'll figure out that the optout package is housed together so it'll be able to package them together for the API 😄

@@ -13,7 +13,7 @@ mkdir -p test_results/latest
echo "Running unit tests and placing results/coverage in test_results/${timestamp} on host..."

# Avoid the db/migrations package since it only contains test code
PACKAGES_TO_COVER=$(go list ./... | egrep -v 'test|mock|db/migrations' | tr "\n" "," | sed -e 's/,$//g')
PACKAGES_TO_COVER=$(go list ./... ./optout | egrep -v 'test|mock|db/migrations' | tr "\n" "," | sed -e 's/,$//g')
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to also update the lint command in the makefile to include this package?

@kyeah kyeah changed the title BCDA-todo: Split opt out utilities into a separate module BCDA-7257: Split opt out utilities into a separate module Jul 21, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #861 (dcc46a6) into master (60d3432) will decrease coverage by 0.02%.
The diff coverage is 94.87%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #861      +/-   ##
==========================================
- Coverage   75.02%   75.00%   -0.02%     
==========================================
  Files          90       92       +2     
  Lines       10401    10393       -8     
==========================================
- Hits         7803     7795       -8     
  Misses       1989     1989              
  Partials      609      609              
Impacted Files Coverage Δ
bcda/models/mock_repository.go 28.41% <0.00%> (ø)
bcda/models/models.go 100.00% <ø> (ø)
bcda/suppression/suppression.go 76.07% <97.29%> (-5.77%) ⬇️
bcda/models/postgres/repository.go 83.62% <100.00%> (ø)
optout/models.go 100.00% <100.00%> (ø)
optout/utils.go 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@kyeah kyeah merged commit e4c1ac6 into master Jul 24, 2023
1 check passed
@kyeah kyeah deleted the kev/suppression_utils branch July 24, 2023 17:54
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.

4 participants