Sk/cog rebase #4489
Sk/cog rebase #4489
Conversation
|
Terraform plan for meta No changes. Your infrastructure matches the configuration.📝 Plan generated in Pull Request Checks #3996 |
|
Terraform plan for dev Error: Unknown ErrorError: Unknown Error
with module.dev.module.newrelic.newrelic_nrql_alert_condition.infected_file_found,
on ../shared/modules/newrelic/alerts.tf line 59, in resource "newrelic_nrql_alert_condition" "infected_file_found":
59: resource "newrelic_nrql_alert_condition" "infected_file_found" {❌ Failed to generate plan in Pull Request Checks #3996 |
|
Lgtm. Is there a quick way to test this works in the app itself? |
+1 to Phil - I was able to get successful test results myself. |
|
Added an advanced test. |
|
|
|
||
| def compute_cog_over( | ||
| federal_awards, submission_status, auditee_ein, auditee_uei, audit_year | ||
| ): |
There was a problem hiding this comment.
This comment is not directly related to the current change, but since the PR modifies this function, I’d like to take the opportunity to clarify some points and potentially simplify the logic for improved readability.
While reviewing this function, I noticed that it checks for None or empty awards at line 36. For context, we compute cog_over as part of dissemination, and based on our current cross-validations, we do not allow reports with no awards to proceed. This makes the check at line 36 seem unnecessary to me.
If anything, I believe it would be better to throw an exception here to indicate an issue with the report. Since I wasn’t part of the discussions that led to this logic, do you recall any context or conversations around this decision? I also noticed a similar check on line 118.
|
The code changes look good to me. After reviewing Matt's diagram and the overall cog_over logic, I think we could consider refactoring the code to align more closely with the diagram. While the current implementation does this to some extent, having the flow in the code mirror the diagram more precisely could make it easier to understand what the code is doing (or is supposed to do) when compared to the diagram. Going through this exercise may also require us to update the diagram (or the code) whenever necessary. This is unrelated to this PR and would be more of a nice-to-have improvement. |
Closes #4481
Changes
How to test
Expected test result
Advanced Test (if interested)
PR Checklist: Submitter
maininto your branch shortly before creating the PR. (You should also be mergingmaininto your branch regularly during development.)git status | grep migrations. If there are any results, you probably need to add them to the branch for the PR. Your PR should have only one new migration file for each of the component apps, except in rare circumstances; you may need to delete some and re-runpython manage.py makemigrationsto reduce the number to one. (Also, unless in exceptional circumstances, your PR should not delete any migration files.)PR Checklist: Reviewer
make docker-clean; make docker-first-run && docker compose up; then rundocker compose exec web /bin/bash -c "python manage.py test"The larger the PR, the stricter we should be about these points.
Pre Merge Checklist: Merger
-/+ resource "null_resource" "cors_header"should be destroying and recreating its self and~ resource "cloudfoundry_app" "clamav_api"might be updating itssha256for thefac-file-scannerandfac-av-${ENV}by default.main.