-
Notifications
You must be signed in to change notification settings - Fork 145
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: Add settings
and plugins
attributes to Project
#7273
Conversation
It seems to simplify quite a few things, and also increased the speed of the tests by about 10%
✅ Deploy Preview for meltano canceled.
|
@WillDaSilva Giving a tentative 👍 on the overall approach, pending a more thorough review in the next couple days. I think it makes sense and reduces a lot of unnecessary complexity. |
Codecov Report
@@ Coverage Diff @@
## main #7273 +/- ##
==========================================
- Coverage 89.57% 89.20% -0.37%
==========================================
Files 288 287 -1
Lines 21497 21326 -171
Branches 2373 2365 -8
==========================================
- Hits 19255 19023 -232
- Misses 1875 1935 +60
- Partials 367 368 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@WillDaSilva this is epic 🙌 I've had a look through and overall this is a huge improvement. Kudos! The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question for my own edification, but looks good. 🚢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillDaSilva This is an incredible refactor!
It also feels faster on some meltano config ...
commands, which makes sense if we're instantiating fewer things and is a good progression towards addressing our Ravioli.
This LGTM, I just have a few questions.
Co-authored-by: Edgar R. M. <edgar@meltano.com>
These changes seem to simplify quite a few things, and also decreased the amount of time it takes to run the tests by approximately 40%, which suggests that these changes improves Meltano's performance.
In a nutshell, since the
project
instance had to be passed around everywhere, and because ofProject.find
caching it was basically a singleton, now we make use of it by doing stuff likeproject.settings
instead ofProjectSettingsService(project)
. This saves us from repeatedly instantiating many "service" classes over and over again (hundreds of times per executions in some cases), which was rather slow. We do need to re-instantiate them to reflect changes to the outside world (e.g. changes to env vars, changes to config files, etc.), so arefresh
method has been added toProject
which causes it to replace itself with a fresh instance, keeping the same object so that any existing references to it get the new "service" instances it carries.I've also removed several unused params that would allow us to provide pre-instantiated "service" classes. Each such param multiplies the complexity of the object because it provides a whole new set of behaviors, almost none of which were tested. If one really needs to pass in a class with one of these parameters, we can add it back in later. Until then, I think it should be removed. You aren't gonna need it.