Skip to content
This repository has been archived by the owner on Aug 24, 2023. It is now read-only.

Breakup main tasks into mockable subroutines, support "v1" or "v2" schema per https://github.com/cisagov/pen-test-portal/pull/3 #51

Closed
wants to merge 30 commits into from

Conversation

S4lt5
Copy link
Contributor

@S4lt5 S4lt5 commented Dec 3, 2022

🗣 Description

To address #15, some of the major actions of the task are broken up into subtasks, and try to give a descriptive outer exception on what went wrong, while passing a trace and inner exception to preserve the "actual" error.

Additionally, this now supports "v1" and "v2" schemas as described @ https://github.com/cisagov/pen-test-portal/pull/3

💭 Motivation and context

I think this helps with readability and testability, since unit tests can now mock up e.g. the s3 test or test them against ephemeral services to make sure things are working correctly.

This is going to be necessary to adapt the changes from the pen-test-portal project as currently testing this task outside of it's native environment is difficult.

🧪 Testing

The new validation and update functions have test coverage now where none existed before.

Mocked database calls are checked to see if the main logic is correctly updating the back end database.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All future TODOs are captured in issues, which are referenced
    in code comments.
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • ❌ These code changes follow cisagov code standards. While the new changes try to adhere to the linting and style rules as best as possible, there are overall issues with this document that I think it would be more harmful than helpful to change at this point for the sake of readability and containing scope of this change
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass. (pre commit hook does not pass, but it succeeds more than it did before!)

✅ Pre-merge checklist

  • Revert dependencies to default branches.
  • Finalize version.

✅ Post-merge checklist

  • Add a tag or create a release.

Matthew Galligan added 30 commits November 29, 2022 10:03
…confusing (since it does not skip a record!)
…ally get pulled in from our exemplar file.
@S4lt5
Copy link
Contributor Author

S4lt5 commented Dec 3, 2022

#49 covers the earlier verison of this.

The only really important comment out of there is:

I refactored out the "finding extraction" function into rdi.extract_findings and added some sanity checks so that my tests would stop crashing. It now should work for "v1" or "v2" data interchangably and will require the following addition into the "live" mapping file:

"id": "RVA ID"

@mcdonnnj
Copy link
Member

@Yablargo Apologies for the complication but you will need to rebuild this pull request because of the merge of #50.

@S4lt5
Copy link
Contributor Author

S4lt5 commented Jan 3, 2023

@Yablargo Apologies for the complication but you will need to rebuild this pull request because of the merge of #50.

No worries, looking at the deltas I think I can just remove the (now unnecessary) workflow changes

@S4lt5
Copy link
Contributor Author

S4lt5 commented Jan 3, 2023

Continued at #55

@S4lt5 S4lt5 closed this Jan 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants