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

Code Reorganization #926

Merged
merged 34 commits into from
Jul 25, 2022
Merged

Code Reorganization #926

merged 34 commits into from
Jul 25, 2022

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Jul 18, 2022

Closes #909
Closes #908
Closes #916

Code Changes

  • move all of the tests to a ctl subdir
  • add fides as an alias entrypoint to the CLI
  • move all of the API logic sans main.py into a ctl subdir

Steps to Confirm

  • automated tests pass
  • loading up and using fides instead of fidesctl works

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated:
    • 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)
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

This PR does some sweeping reorganization of the repository in preparation for eventually combining with the fidesops repository. These changes are designed to minimize the amount of conflicts we'll have to deal with and allow us to keep the products semi-partitioned at first to make it easier to execute the repo merge. The changes in detail are as follows

  • Move all of our tests into a tests/ctl/ dir so we can eventually put ops tests into tests/ops and run them independently by changing the testing path
  • move all of the ctl API logic into fidesctl.api.ctl so that we can also partition ops API logic into fidesctl.api.ops (note: main.py is remaining at the top-level fidesctl.api. since we'll want ops and ctl to run on the same webserver
  • move the core and connectors logic into a distinct ctl folder for more partitioning (cli and api are left at the top-level since they'll be shared with ops from the get-go)

@ThomasLaPiana ThomasLaPiana self-assigned this Jul 18, 2022
@ThomasLaPiana
Copy link
Contributor Author

getting a weird bug here:

fides> fides
Usage: fides [OPTIONS] COMMAND [ARGS]...

  The parent group for the Fidesctl CLI.

Options:
  --version               Show the version and exit.   
  -f, --config-path TEXT  Path to a configuration      
                          file. Use 'fidesctl view-    
                          config' to print the
                          config. Not compatible with  
                          the 'fidesctl webserver'     
                          subcommand.
  --local                 Run in 'local_mode'. This    
                          mode doesn't make API calls  
                          and can be used without the  
                          API server/database.
  -h, --help              Show this message and exit.  

Commands:
  annotate   Annotate fidesctl resource types
  apply      Validate local manifest files and...      
  db         Database utility commands
  delete     Delete a resource on the server.
  evaluate   Compare your System's Privacy...
  export     Export fidesctl resource types
  generate   Generate fidesctl resource types
  get        View a resource from the server as...     
  init       Initializes a Fidesctl instance,...       
  ls         Get a list of all resources of this...    
  parse      Reads the resource files that are...      
  scan       Scan external resource coverage...        
  status     Sends a request to the Fidesctl API...    
  sync       Update local resource files by...
  view       View various resources types.
  webserver  Starts the fidesctl API server...
Traceback (most recent call last):
  File "C:\Users\tlapiana\miniconda3\Scripts\fides-script.py", line 33, in <module>
    sys.exit(load_entry_point('fidesctl', 'console_scripts', 'fides')())
  File "C:\Users\tlapiana\miniconda3\lib\site-packages\click\core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "C:\Users\tlapiana\miniconda3\lib\site-packages\click\core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "C:\Users\tlapiana\miniconda3\lib\site-packages\click\core.py", line 1635, in invoke
    rv = super().invoke(ctx)
  File "C:\Users\tlapiana\miniconda3\lib\site-packages\click\core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)     
  File "C:\Users\tlapiana\miniconda3\lib\site-packages\click\core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "C:\Users\tlapiana\miniconda3\lib\site-packages\click\decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)   
  File "c:\users\tlapiana\documents\github\fides\src\fides\cli\__init__.py", line 106, in cli
    production_version=version(APP),
  File "C:\Users\tlapiana\miniconda3\lib\importlib\metadata.py", line 530, in version
    return distribution(distribution_name).version     
  File "C:\Users\tlapiana\miniconda3\lib\importlib\metadata.py", line 503, in distribution
    return Distribution.from_name(distribution_name)   
  File "C:\Users\tlapiana\miniconda3\lib\importlib\metadata.py", line 177, in from_name
    raise PackageNotFoundError(name)
importlib.metadata.PackageNotFoundError: fides

but if I run the command from another directory it works just fine, thinking I have some sort of cruft in my system that is preventing this from working properly

GitHub> fides
Usage: fides [OPTIONS] COMMAND [ARGS]...

  The parent group for the Fidesctl CLI.

Options:
  --version               Show the version and exit.   
  -f, --config-path TEXT  Path to a configuration      
                          file. Use 'fidesctl view-    
                          config' to print the
                          config. Not compatible with  
                          the 'fidesctl webserver'     
                          subcommand.
  --local                 Run in 'local_mode'. This    
                          mode doesn't make API calls  
                          and can be used without the  
                          API server/database.
  -h, --help              Show this message and exit.  

Commands:
  annotate   Annotate fidesctl resource types
  apply      Validate local manifest files and...      
  db         Database utility commands
  delete     Delete a resource on the server.
  evaluate   Compare your System's Privacy...
  export     Export fidesctl resource types
  generate   Generate fidesctl resource types
  get        View a resource from the server as...     
  init       Initializes a Fidesctl instance,...       
  ls         Get a list of all resources of this...    
  parse      Reads the resource files that are...      
  scan       Scan external resource coverage...        
  status     Sends a request to the Fidesctl API...    
  sync       Update local resource files by...
  view       View various resources types.
  webserver  Starts the fidesctl API server...

@ThomasLaPiana
Copy link
Contributor Author

ThomasLaPiana commented Jul 19, 2022

@sanders41 any ideas here on this bug? haven't seen this one before and it seems obscure

when I do a pip list it doesn't show up, interestingly enough...with how setup.py is configured it should show fides as a package as well

other note:

fides> python -c "import fides" works just fine though

other other note:

if I update the package name to fides everything works just fine 🤷

@ThomasLaPiana
Copy link
Contributor Author

couldn't figure out the issue so I reduced the scope of the ticket to leave src/fidesctl as-is

@sanders41
Copy link
Contributor

@ThomasLaPiana after you made the changes to setup.py and changed the module name did you run pip install -e .? I don't think it will automatically pick up these changes without that.

I looked at how you had things pre-reverting the module name and as far as I can tell it looks correct.

@ThomasLaPiana
Copy link
Contributor Author

@ThomasLaPiana after you made the changes to setup.py and changed the module name did you run pip install -e .? I don't think it will automatically pick up these changes without that.

I looked at how you had things pre-reverting the module name and as far as I can tell it looks correct.

yep I was doing a pip uninstall <package>, then pip install -e . as well as getting rid of any possible build artifacts I could find

I think it is a weird case of some sort related to click and how it expects packages to be listed? idk, I've moved on for now, can revisit later when it becomes more important

@ThomasLaPiana ThomasLaPiana marked this pull request as ready for review July 19, 2022 18:04
@ThomasLaPiana ThomasLaPiana requested review from a team July 19, 2022 18:04
@ThomasLaPiana
Copy link
Contributor Author

please forgive me for the files changed number, I promise I just moved things around and updated import statements 🙏

.gitignore Outdated Show resolved Hide resolved
@ThomasLaPiana
Copy link
Contributor Author

aite I gotta fix these merge conflicts...please review after they're fixed

@ThomasLaPiana ThomasLaPiana marked this pull request as draft July 23, 2022 12:04
@ThomasLaPiana ThomasLaPiana marked this pull request as ready for review July 25, 2022 01:00
Copy link
Contributor

@SteveDMurphy SteveDMurphy left a comment

Choose a reason for hiding this comment

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

I think the only way to dive into this was to really dive in... I ran a bunch of cli commands (both from the fidesctl and fides entrypoint) as well as hit multiple API endpoints and I think it all checks out! Holding off merging anything else you reviewed this weekend and figure we can sort through any issues over the next day or so if that works. Nice work and thanks for working through so many merge conflicts!

@ThomasLaPiana
Copy link
Contributor Author

@SteveDMurphy appreciate it! I'll go ahead and merge now so people can get stuck in right away on Monday morning :)

@ThomasLaPiana ThomasLaPiana merged commit 1c26a67 into main Jul 25, 2022
@ThomasLaPiana ThomasLaPiana deleted the ThomasLaPiana-generic-reorg branch July 25, 2022 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants