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

Add cve option in findings #1106

Merged
merged 5 commits into from May 7, 2019

Conversation

@dr3dd589
Copy link
Member

dr3dd589 commented May 3, 2019

related to #973
Updated db_migration.

  • Your code is flake8 compliant (DefectDojo's code isn't currently flake8 compliant, but we're trying to correct that.)
  • If this is a new feature and not a bug fix, you've included the proper documentation in the ReadTheDocs documentation folder. https://github.com/DefectDojo/Documentation/tree/master/docs or provide feature documentation in the PR.
  • Model changes should include the necessary migrations in the dojo/dd_migrations folder.
  • Add applicable tests to the unit tests.
@efficiosoft

This comment has been minimized.

Copy link
Contributor

efficiosoft commented May 6, 2019

This has to be rebased onto dev and the migration to be regenerated again I fear, because it contains the change to hash_code for which a migration has already been created in dev.

Please unapply the migration locally first before rebasing and deleting the old one:

$ ./manage.py migrate dojo 0001_initial
# Rebase.
$ git rm <your migration>
$ ./manage.py makemigrations dojo

Regards
Robert

@dr3dd589 dr3dd589 force-pushed the dr3dd589:add_cve_option_in_findings branch from 5d884c3 to 87cce27 May 6, 2019
@dr3dd589

This comment has been minimized.

Copy link
Member Author

dr3dd589 commented May 6, 2019

done, thanks!

@valentijnscholten

This comment has been minimized.

Copy link
Contributor

valentijnscholten commented May 6, 2019

Great, maybe rename the migrations file to something like 003_cve_field or similar?

Copy link
Contributor

valentijnscholten left a comment

rename migrations file

@dr3dd589

This comment has been minimized.

Copy link
Member Author

dr3dd589 commented May 6, 2019

done, thanks! @valentijnscholten

Copy link
Contributor

valentijnscholten left a comment

Thanks, but the 0003 was just a suggestion. Looking at 'dev' it should be 0004. Not sure it matters but two files with 0003 would be confusing 😃

@efficiosoft

This comment has been minimized.

Copy link
Contributor

efficiosoft commented May 6, 2019

Well, the naming would just cause an usability issue, making it impossible to abbreviate migration names when using the migrate command.

However, what's important are the dependencies set inside the migration file. The migration in question currently depends on 0002_something, which would create a branch in django's migration dependency graph since there's another one (0003_test_title) depending on that migration as well.

@dr3dd589 So it should work when merged as is right now, but another unmigrate/rebase/makemigrations (the steps I already described earlier today) would make it more sane and maintain a linear migration path without branches. Maybe I could kindly ask you to rebase again? Thanks!

@aaronweaver Dealing with these migrations might seem like a lot of work at the moment, but quite some db changes have queued up and need to be persisted in order. I'm sure this will settle soon and we'll all benefit from a clean migration chain.

@dr3dd589 dr3dd589 force-pushed the dr3dd589:add_cve_option_in_findings branch from c4d8178 to ca0b47e May 6, 2019
@dr3dd589

This comment has been minimized.

Copy link
Member Author

dr3dd589 commented May 6, 2019

It means I have to make these changes in PR whenever anyone updates DB file until this pr gets merged!!

@aaronweaver

This comment has been minimized.

Copy link
Collaborator

aaronweaver commented May 7, 2019

@dr3dd589, @valentijnscholten we good to merge now? All tests look clear. @dr3dd589 thanks for your patience, the migrations included does add additional overhead.

@efficiosoft

This comment has been minimized.

Copy link
Contributor

efficiosoft commented May 7, 2019

@dr3dd589 I double-checked my statement from yesterday again. Migration dependencies actually need to be liniarized, meaning each django app has to have only one migration at the top. This would have had to be merged later manually [1], so it's good you rebased again.

[1] https://docs.djangoproject.com/en/2.2/topics/migrations/#version-control

Copy link
Contributor

valentijnscholten left a comment

Approved, thanks for your patience and help.

@dr3dd589

This comment has been minimized.

Copy link
Member Author

dr3dd589 commented May 7, 2019

@dr3dd589, @valentijnscholten we good to merge now? All tests look clear. @dr3dd589 thanks for your patience, the migrations included does add additional overhead.

@aaronweaver yes I think so!

@aaronweaver aaronweaver merged commit d3d43cc into DefectDojo:dev May 7, 2019
4 checks passed
4 checks passed
AccessLint Review complete
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - components/package.json (aaronweaver (GitHub marketplace)) No manifest changes detected
security/snyk - requirements.txt (aaronweaver (GitHub marketplace)) No manifest changes detected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.