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

refactor(core) Make TRAM an installable package #155

Merged
merged 4 commits into from
Feb 21, 2022

Conversation

mehaase
Copy link
Contributor

@mehaase mehaase commented Feb 17, 2022

This looks like a big commit, but its actually one main change and a bunch of small changes to support that main change.

  • add setup.cfg so that TRAM can be installed as a package. Now TRAM doesn't need to be manually added to PYTHONPATH, and also python src/tram/manage.py is now aliased to tram so you can do things like tram runserver.
  • Update documentation and build artifacts to use tram ... shortcut instead of python src/tram/manage.py ...
  • Move src/tram/tram/* to src/tram/*. This large rename causes the bulk of what you see in this commit, but I think its a cleaner layout, it's what developers will expect when they check out the project for the first time, and its a more natural fit for an installable package to be laid out this way.

Setting this as WIP until we can discuss in the tech lead group.

closes #16

This looks like a big commit, but its actually one main change and a
bunch of small changes to support that main change.

* add setup.cfg so that TRAM can be installed as a package. Now TRAM
  doesn't need to be manually added to PYTHONPATH, and also `python
  src/tram/manage.py` is now aliased to `tram` so you can do things like
  `tram runserver`.
* Update documentation and build artifacts to use `tram ...` shortcut
  instead of `python src/tram/manage.py ...`
* Move src/tram/tram/* to src/tram/*. This large rename causes the bulk
  of what you see in this commit, but I think its a cleaner layout, it's
  what developers will expect when they check out the project for the
  first time, and its a more natural fit for an installable package to be
  laid out this way.
@mehaase mehaase requested a review from emmanvg February 17, 2022 16:14
@mehaase mehaase self-assigned this Feb 17, 2022
@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #155 (3a6429d) into master (01c8912) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #155   +/-   ##
=======================================
  Coverage   95.63%   95.63%           
=======================================
  Files           9        9           
  Lines         848      848           
=======================================
  Hits          811      811           
  Misses         37       37           
Impacted Files Coverage Δ
src/tram/admin.py 100.00% <ø> (ø)
src/tram/management/commands/attackdata.py 97.01% <ø> (ø)
src/tram/management/commands/pipeline.py 100.00% <ø> (ø)
src/tram/ml/base.py 92.03% <ø> (ø)
src/tram/models.py 100.00% <ø> (ø)
src/tram/report/docx.py 100.00% <ø> (ø)
src/tram/serializers.py 95.16% <ø> (ø)
src/tram/urls.py 100.00% <ø> (ø)
src/tram/views.py 93.02% <ø> (ø)

Continue to review full report at Codecov.

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

* Fix Sonar Cloud warnings.
* Test/fix docker build.
Dockerfile Outdated Show resolved Hide resolved
* Fix Docker lint errors
* Attempt to fix the black/flake8 lint errors that occur only in CI and
  not in local. The linters should be ignoring migrations.
@mehaase
Copy link
Contributor Author

mehaase commented Feb 18, 2022

@m3mike The super linter is throwing errors for the Django migrations, but pyproject.toml is supposed to ignore the migrations. Have you seen this before?

@m3mike
Copy link
Contributor

m3mike commented Feb 18, 2022

@m3mike The super linter is throwing errors for the Django migrations, but pyproject.toml is supposed to ignore the migrations. Have you seen this before?

Yes I have, and yes it's annoying. If I remember right, since from a git perspective the file was changed, the file gets included in a changeset passed directly to the linters. And when a file list is presented to the linters, it can override the ignore regex. I'll add a suggestion thing to the ci check that should fix it.

@mehaase
Copy link
Contributor Author

mehaase commented Feb 18, 2022

Yes I have, and yes it's annoying. If I remember right, since from a git perspective the file was changed, the file gets included in a changeset passed directly to the linters. And when a file list is presented to the linters, it can override the ignore regex. I'll add a suggestion thing to the ci check that should fix it.

I was wondering if that was the issue, and I noticed this config VALIDATE_ALL_CODEBASE: false. What if I set that to true, then fix all of the issues it raises? (Can't be that many lint issues at this point since you linted the whole repo last week.)

* Using M3's suggestion to exclude migrations from super linter.
@m3mike
Copy link
Contributor

m3mike commented Feb 18, 2022

Yes I have, and yes it's annoying. If I remember right, since from a git perspective the file was changed, the file gets included in a changeset passed directly to the linters. And when a file list is presented to the linters, it can override the ignore regex. I'll add a suggestion thing to the ci check that should fix it.

I was wondering if that was the issue, and I noticed this config VALIDATE_ALL_CODEBASE: false. What if I set that to true, then fix all of the issues it raises? (Can't be that many lint issues at this point since you linted the whole repo last week.)

Actually never tried that for this case but it could work, typically just ended up overriding the merge block on github and documented why in these cases. If you did go that route, you'd probably want to switch it back for normal use, though there isn't that much code so it might not increase CI time too much.

@sonarcloud
Copy link

sonarcloud bot commented Feb 18, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 53 Code Smells

No Coverage information No Coverage information
2.8% 2.8% Duplication

@mehaase
Copy link
Contributor Author

mehaase commented Feb 18, 2022

FILTER_REGEX_EXCLUDE is exactly what I was looking for and the build is green again. Thanks @m3mike!

@mehaase mehaase changed the title WIP refactor(core) Make TRAM an installable package refactor(core) Make TRAM an installable package Feb 18, 2022
@mehaase mehaase requested a review from emmanvg February 18, 2022 20:14
@emmanvg emmanvg added this to the v1.1.3 milestone Feb 21, 2022
Copy link
Contributor

@emmanvg emmanvg left a comment

Choose a reason for hiding this comment

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

TRAM as a installable library works as expected. Linter issues resolved.

@emmanvg emmanvg merged commit 575ff7a into master Feb 21, 2022
@emmanvg emmanvg deleted the make-tram-installable branch February 21, 2022 12:27
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.

TRAM is a library
3 participants