Skip to content

Restructure Django apps#119

Closed
jerbob wants to merge 224 commits intomasterfrom
feature/app-restructure
Closed

Restructure Django apps#119
jerbob wants to merge 224 commits intomasterfrom
feature/app-restructure

Conversation

@jerbob
Copy link
Member

@jerbob jerbob commented Jun 9, 2021

This PR contains a bunch of sweeping changes to the project's app names and scopes. The goal here is to get rid of any redundant apps, and merge any apps whose scopes intersect.

@codecov
Copy link

codecov bot commented Jun 9, 2021

Codecov Report

Merging #119 (b801294) into master (35b9f20) will decrease coverage by 0.05%.
The diff coverage is 97.77%.

❗ Current head b801294 differs from pull request most recent head d2a33d0. Consider uploading reports for the commit d2a33d0 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
- Coverage   94.40%   94.35%   -0.06%     
==========================================
  Files         107      106       -1     
  Lines        4754     4623     -131     
  Branches      272      251      -21     
==========================================
- Hits         4488     4362     -126     
+ Misses        223      221       -2     
+ Partials       43       40       -3     
Impacted Files Coverage Δ
src/andromeda/apps.py 100.00% <ø> (ø)
src/andromeda/serializers.py 100.00% <ø> (ø)
src/challenge/apps.py 100.00% <ø> (ø)
src/challenge/signals.py 100.00% <ø> (ø)
src/challenge/sql.py 100.00% <ø> (ø)
src/challenge/tests/mixins.py 100.00% <ø> (ø)
src/challenge/tests/test_utils.py 100.00% <ø> (ø)
src/config/apps.py 100.00% <ø> (ø)
src/config/backends.py 80.00% <ø> (-9.10%) ⬇️
src/config/config.py 100.00% <ø> (ø)
... and 122 more

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 35b9f20...d2a33d0. Read the comment docs.

@jerbob jerbob marked this pull request as ready for review June 9, 2021 17:10
@jerbob jerbob marked this pull request as draft June 9, 2021 17:11
@jerbob jerbob marked this pull request as ready for review June 10, 2021 10:35
@jerbob jerbob marked this pull request as draft June 10, 2021 10:36
@0xAda 0xAda force-pushed the feature/app-restructure branch from 94cc99b to fefdb53 Compare June 10, 2021 17:05
@jerbob jerbob mentioned this pull request Jun 11, 2021
6 tasks
@jchristgit jchristgit self-requested a review August 18, 2021 23:47
@0xAda 0xAda assigned thebeanogamer and unassigned 0xAda Aug 18, 2021
@jerbob jerbob force-pushed the feature/app-restructure branch from 232bdad to d2a33d0 Compare October 26, 2021 19:19
@jchristgit
Copy link
Collaborator

Hello @jerbob, just checking in to see how you're doing. Do you need help with this PR?

@jchristgit jchristgit removed their request for review December 11, 2021 12:50
@jchristgit jchristgit assigned 0xAda and jerbob and unassigned thebeanogamer Jan 14, 2022
@jchristgit jchristgit added the enhancement New feature or request label Jan 15, 2022
@jchristgit
Copy link
Collaborator

@jerbob do we need the migrations to move data over? We only ever run the app for a single event anyways.

@jchristgit jchristgit self-requested a review January 20, 2022 21:36
Copy link
Collaborator

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

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

Jeremiah,

While I highly appreciate the amount of effort that has flown into this pull
request, in its current state, it is not possible to actually review the code,
as I am plagued with local setup errors. I understand it's hard to keep a pull
request of this size in sync with the master branch, but I cannot adjust half
the project when testing it locally. I've encountered a number of issues whilst
trying to set this up:

  • poetry.lock was not working properly, and caused poetry to crash with an
    error. Running poetry lock fixed it.

  • After building the project in docker-compose and starting it, it crashed
    multiple times. For the following reasons, in no particular order:

    • Unexpected indents
    • Undefined imports
    • Missing migrations
  • I tried to fix the missing imports by using flake8, or at least find them,
    but flake8 also crashes out with an error, I assume due to a configuration
    error:

flake8.exceptions.FailedToLoadPlugin: Flake8 failed to load plugin "pylint" due
to cannot import name 'MergedConfigParser' from 'flake8.options.config' (...).

I believe the majority of these problems is caused by the recent merge with
option -X ours. I understand it's annoying to fix a big merge conflict, but
perhaps we should roll the merge back, and try to resolve it one-by-one?

@0xAda 0xAda force-pushed the feature/app-restructure branch from 221a28b to d2a33d0 Compare January 20, 2022 22:28
@jerbob
Copy link
Member Author

jerbob commented Jan 22, 2022

Johannes,

While I highly appreciate your need to check in on unfinished "Draft" PRs
as the RACTF Principal Software Engineer, please note that this PR is
marked as a "Draft". PRs that are marked as a "Draft" are generally not yet
ready for review, which is why I've explicitly marked this PR as a "Draft"
(it is not yet ready for review). When it is ready for a review (i.e.
"Ready for review"), I will mark it as "Ready for review" which implies
that the PR in question is ready to be reviewed (it is not). You're right
that a lot of work's been put into this "Draft" PR, but it's still far from
done (i.e. a "Draft"), which is why I haven't moved it from "Draft" to
"Ready for review" just yet. I will remove this PR from "Draft" and into
"Ready for review" once the PR is viable for a code review.

Your unsolicited code review on this "Draft" (not ready for a code review)
PR is highly appreciated, but the points raised are all in reference to
aspects of this PR that will change long before I move the PR from "Draft"
(not ready for a review) to "Ready for review" (ready for a review). In
particular, issues raised by a faulty merge commit that can be rectified by
reverting and rebasing manually.

You can find out more about "Draft" pull requests in this blog post:
https://github.blog/2019-02-14-introducing-draft-pull-requests

You can find out more about the etymology of the word "Draft" here:
https://www.etymonline.com/word/draft

You can find out more about the etymology of the word "unsolicited" here:
https://www.etymonline.com/word/unsolicited

@jerbob
Copy link
Member Author

jerbob commented Jan 22, 2022

As an addendum, I will click this "Ready for review" button below my
comment when the pull request is no longer a "Draft" and is ready for a
review (it is not):
image

Thank you for your service, and I look forward to ignoring more of your
reviews in the future.

@jchristgit
Copy link
Collaborator

jchristgit commented Jan 22, 2022

I'm sorry, Jeremiah. I will take my review down immediately, and implement
measures to prevent this from happening in the future. I will not leave
unsolicited reviews on any Really Awesome CTF Ltd. repositories in the future,
and I will ensure that any review I leave is friendly and welcoming as opposed
to being this hostile. I apologize for my actions.

@jerbob
Copy link
Member Author

jerbob commented Jan 22, 2022

Johannes,

thanks

@0xAda
Copy link
Contributor

0xAda commented Apr 15, 2023

😢

@jchristgit
Copy link
Collaborator

As a longtime fan of the RACTF backend, I'm sad to see this go.

When I was a child, I loved learning about new developments of the RACTF
backend app restructure pull request. Whenever I returned from school, it was
the first question I wanted answered. The lack of updates was always both
interesting and sad to me, and I was always hoping for a timely update on such
an important topic.

As I grew older, my childlike fascination with the lack of updates on the pull
request grew into a more stoic lifeform, and the overall stagnation of the pull
request slowly started to affect my own life. Looking for a way to get out, I
learnt Python programming. I am sad that to this day, I have not been able to
provide any net benefit to this pull request, despite my ability to read basic
Python code and even perhaps write a bit with the help of modern assistant
tooling.

Jeremiah, thank you for all your hard work. I can understand why you've chosen
this step, and let it be known that your cries for aid in the deep trenches of
refactoring this code have not been left unheard. I can assure you that life
always finds a way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants