Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Reorganize application code #1058

Merged
merged 13 commits into from
Aug 11, 2022
Merged

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Aug 10, 2022

Purpose

Reorganize the code to make the repo merger easier

Changes

  • nest the application code down to match the fidesctl layout

Checklist

  • Update CHANGELOG.md file
    • Merge in main so the most recent CHANGELOG.md file is being appended to
    • Add description within the Unreleased section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.
    • Add a link to this PR at the end of the description with the PR number as the text. example: #1
  • Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
  • If docs updated (select one):
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Good unit test/integration test coverage
  • This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
  • The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #1005

@ThomasLaPiana ThomasLaPiana self-assigned this Aug 10, 2022
@ThomasLaPiana
Copy link
Contributor Author

@PSalant726 I'm assuming that the CodeQL warning is getting triggered because of the new Path?

@ThomasLaPiana ThomasLaPiana marked this pull request as ready for review August 10, 2022 11:32
@ThomasLaPiana
Copy link
Contributor Author

documentation should have gotten updated here with the sweeping find and replaces, I'll take a look at what changed to double check though

@sanders41
Copy link
Contributor

sanders41 commented Aug 10, 2022

One thing I found in testing that I haven't found a reason for yet. I am having an issue with both nox -s dev and docker-compose up, when I try to login in from the admin-ui front end I get blocked. No log ever shows up on the server end as an attempted login. I compared the route the login page is using to the FastAPI docs route and everything looks correct. Maybe the CORS settings aren't getting applied?

Update:
I found the reason. It is browser specific. Things don't seem to work with Brave and it is unrelated to this update.

@PSalant726
Copy link
Contributor

I'm assuming that the CodeQL warning is getting triggered because of the new Path?

Looks like a false positive because our variables contain the literal word "secret" ? My guess is that we dismissed a similar warning before the path changed, and now CodeQL is picking it up again.

The logging is not printing anything sensitive, so I think the warning can be dismissed. Someone with more context for this code should weigh in, just to be safe.

Copy link
Contributor

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

There is one thing in the docs part that looks like it needs updating. Then every question after that is the same, are the .tmp files supposed to be there?

So it looks like a lot but the answer will be the same for all but one of the comments, so no need to reply directly to every one of them.

@ThomasLaPiana
Copy link
Contributor Author

@sanders41 Aha, I know what happened with all of those tmp files. The find & replace took a toll on my computer (100+ files and automatic formatting) and VS code crashed, I'm assuming where those all came from, as they were in limbo waiting to be saved or something. Will delete them and update the docs page

@ThomasLaPiana
Copy link
Contributor Author

@sanders41 I made some more thorough updates, would appreciate another review! 🙏

Copy link
Contributor

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

lgtm. A crash would do it with the tmp files. Sounds like it was a fun update 😄

@seanpreston this one looks read to merge to me whenever you think it would be a good time.

@seanpreston seanpreston merged commit db713b1 into main Aug 11, 2022
@seanpreston seanpreston deleted the ThomasLaPiana-reorg-appliation-code branch August 11, 2022 14:58
sanders41 added a commit that referenced this pull request Sep 22, 2022
* Reorganize application code

* first round of moving everything and updating the import paths

* checkpoint, more path updates

* more path updates

* fix imports

* fix isort

* fix mypy, isort and setup.py issues

* fix unsafe checks build step running on any label

* update the changelog

* delete temp files

* Update docs/fidesops/docs/development/contributing_details.md

Co-authored-by: Paul Sanders <psanders1@gmail.com>

* update more file references

Co-authored-by: Paul Sanders <psanders1@gmail.com>
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.

Reorganize the API logic to make repo merger easier
4 participants