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
feat: Just-in-time plugin installation #8482
base: main
Are you sure you want to change the base?
feat: Just-in-time plugin installation #8482
Conversation
✅ Deploy Preview for meltano ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
93cc0b1
to
7b9bf26
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8482 +/- ##
==========================================
- Coverage 91.83% 91.73% -0.11%
==========================================
Files 245 245
Lines 19337 19353 +16
Branches 2152 2150 -2
==========================================
- Hits 17758 17753 -5
- Misses 1306 1326 +20
- Partials 273 274 +1 ☔ View full report in Codecov by Sentry. |
We probably want to suppress install progress to stdout from
|
Yeah, I'd be more than OK with starting to unify our output away from |
601322c
to
44680fa
Compare
Think I have covered all relevant commands here. Wondering if we want a new
I haven't yet addressed install progress/status as logs, but the output is suppressed for |
ad6e137
to
89f1848
Compare
Should this be added for |
If it's opt-in, probably yes 👍 |
ADD = "add" | ||
CONFIG_TEST = "config test" | ||
EL = "el" | ||
ELT = "elt" | ||
INSTALL = "install" | ||
INVOKE = "invoke" | ||
RUN = "run" | ||
SELECT = "select" | ||
TEST = "test" | ||
UPGRADE = "upgrade" |
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.
Should we collect all these newly-added install reasons under an aggregate PREREQUISITE
reason, or something like that?
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.
Yeah, PREREQUISITE
or something like JIT
or similar seems like a good option
@@ -323,7 +317,13 @@ async def install_plugin_async( | |||
), | |||
) | |||
|
|||
if not plugin.is_installable() or self._is_mapping(plugin): | |||
is_installed = self.project.plugin_dir(plugin, "venv", make_dirs=False).exists() |
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.
This is a very basic check to determine whether or not to install based on the presence of the plugin virtual environment directory. Is there anything more intelligent we can do here to satisfy the stale plugin requirement from #2089? #6416 (comment) and #2651 seem relevant.
Implements just-in-time install for the following commands:
invoke
run
el
/elt
test
config <plugin> test
select --list