feat(core): pre-fetch provider tools and make tools a native feature #1858

edvald · 2020-06-04T09:47:27Z

What this PR does / why we need it:

This adds the notion of tools to the plugin framework, and a command to
allow using and pre-fetching these tools.

The primary motivation was to allow pre-fetching the tools as part of
our container builds, as well as to make it easy for users to do the
same in their custom CI setups with Garden.

The garden tools command is also handy since it allows users to easily
use the exact same tools as Garden's providers fetch and use.

To pre-fetch the tools, we add a garden util fetch-tools command,
which fetches all tools for the configured providers in the current
project, or for all registered providers if the --all flag is set.

Notes:

We now always fetch the docker binary on use, instead of checking for
its existence locally. This avoids some code complexity and makes sure
we always use the expected version. The downside should be mitigated
by the pre-fetching in built containers, and the fetch-tools command.
We really need to reduce the number of tools and versions we bundle
before we release 0.12. The container image sizes are too large atm.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Any feedback on the design/usability here would be good, and maybe give it a spin, see if it all works as expected.

eysi09

Had a few comments/questions, but otherwise looks great!

Some additional questions:

Are we verifying anywhere, either explicitly or implicitly, that the tool remote URLs are correct across all platforms?

Is there any concern that the tools will conflict with previous installations? For example, if you have two Docker installations or install a tool via Chocolate and again via Garden.

You mentioned the images where too bloated, how do we plan on addressing that?

docs/reference/commands.md

eysi09 · 2020-06-09T08:55:37Z

docs/reference/commands.md

+conflicts.
+
+When there are name conflicts and a plugin name is not specified, the preference is
+for defined by configured providers in the current project (if applicable), and then


Should be: the preference is defined by the dependency order of configured providers, right?

In any case the for shouldn't be there.

Actually it's supposed to be "the preference is for tools defined by configured providers". I'll fix and likely rephrase for better clarity.

eysi09 · 2020-06-09T09:00:08Z

garden-service/src/analytics/analytics.ts

@@ -126,15 +125,12 @@ export class AnalyticsHandler {
  private ciName = ci.name
  private systemConfig: SystemInfo
  private isCI = ci.isCI
-  private sessionId: string
+  private sessionId: string | null


Why are we changing the behaviour? Shouldn't the class throw if the session id is missing instead of just failing silently?

I kept getting errors in unit tests. As far as I can tell this case (null sessionId) only comes up there. I'm open to a separate fix for that, but I couldn't quite grok why this was how it was so I'd ask you or @thsig to investigate.

Yeah the sessionId appears to be created in the CLI class and passed to BufferedEventStream and the Garden class. It doesn't look its really used by the Garden class though, so perhaps we could just pass it directly to the AnalyticsHandler class.

In any case, I don't think we should change the behaviour of the AnalyticsHandler class to accommodate the unit tests.

If the AnalyticsHandler is never supposed to receive a null sessionId, it should imo be caught at compile time. I'm unclear on why there's a runtime guard there then.

To be clear, I wasn't trying to get my tests to pass, other unrelated tests kept failing. I can revert this, but then we'd need a separate PR to fix the failing tests.

eysi09 · 2020-06-09T09:09:33Z

garden-service/src/analytics/analytics.ts

@@ -247,7 +243,7 @@ export class AnalyticsHandler {
   * Used internally to check if a users has opted-in or not.
   */
  private analyticsEnabled(): boolean {
-    if (process.env.GARDEN_DISABLE_ANALYTICS === "true") {
+    if (!this.sessionId || process.env.GARDEN_DISABLE_ANALYTICS === "true") {


See my comment above. A missing session ID shouldn't signify a disabled analytics imo.

Is there a valid scenario where the session ID is null but we want events to be sent?

IMO the issue is that the sessionId shouldn't be null to begin with. That's why before this commit, the AnalyticsHandler would just throw if it was missing.

eysi09 · 2020-06-09T09:12:28Z

garden-service/src/commands/tools.ts

+  help = "Access tools included by providers."
+  cliOnly = true
+
+  // FIXME: We need this while we're still resolving providers in the AnalyticsHandler


Where are we doing that? When generating the metadata we call getRawModuleConfigs.

We do now, not when this was written originally. You fixed that quite recently. Comment is out of date, but the flag remains necessary, so I'll remove the comment.

garden-service/src/commands/tools.ts

garden-service/src/commands/util.ts

eysi09 · 2020-06-09T09:58:51Z

garden-service/src/util/ext-tools.ts

+  cwd?: string
+  env?: { [key: string]: string }
+  log: LogEntry
+  timeout?: number


This is seconds, right? Should we perhaps make it explicit my naming it timeoutSec? Or stay consistent with execa and use milliseconds as well, and name the variable timeoutMs?

Or do have we consistently use seconds in throughout the codebase?

This is unchanged from previous code, but yeah I wouldn't mind making the unit explicit everywhere.

garden-service/src/util/ext-tools.ts

edvald · 2020-06-09T16:58:47Z

Are we verifying anywhere, either explicitly or implicitly, that the tool remote URLs are correct across all platforms?

That would be good. We haven't done that so far, but it would be feasible to create an integ test suite that does that, or something in CI.

Is there any concern that the tools will conflict with previous installations? For example, if you have two Docker installations or install a tool via Chocolate and again via Garden.

Not really, those won't conflict. But we will indeed fetch Docker even if it's already installed.

You mentioned the images where too bloated, how do we plan on addressing that?

I added a couple of to-do items in our 0.12 checklist, for dropping old tools that we don't aim to support anymore. That's a big chunk. Then there are smaller improvements we could make, e.g. make it possible to drop files that we don't use in archives we extract.

eysi09 · 2020-06-09T17:53:02Z

Thanks for the answers, make sense. Do you want to look into the tests now, or just the next time we actually tamper with the tool URLs?

edvald · 2020-06-09T17:56:03Z

We already test most of them implicitly in container builds, integ/e2e tests and Windows tests, so I'm not sure I'd want to get into more explicit tool tests. Until a new reason comes up, that is.

This adds the notion of tools to the plugin framework, and a command to allow using and pre-fetching these tools. The primary motivation was to allow pre-fetching the tools as part of our container builds, as well as to make it easy for users to do the same in their custom CI setups with Garden. The `garden tools` command is also handy since it allows users to easily use the exact same tools as Garden's providers fetch and use. To pre-fetch the tools, we add a `garden util fetch-tools` command, which fetches all tools for the configured providers in the current project, or for all registered providers if the `--all` flag is set. Notes: - We now always fetch the docker binary on use, instead of checking for its existence locally. This avoids some code complexity and makes sure we always use the expected version. The downside should be mitigated by the pre-fetching in built containers, and the fetch-tools command. - We really need to reduce the number of tools and versions we bundle before we release 0.12. The container image sizes are too large atm.

…1858) * feat(core): pre-fetch provider tools and make tools a native feature This adds the notion of tools to the plugin framework, and a command to allow using and pre-fetching these tools. The primary motivation was to allow pre-fetching the tools as part of our container builds, as well as to make it easy for users to do the same in their custom CI setups with Garden. The `garden tools` command is also handy since it allows users to easily use the exact same tools as Garden's providers fetch and use. To pre-fetch the tools, we add a `garden util fetch-tools` command, which fetches all tools for the configured providers in the current project, or for all registered providers if the `--all` flag is set. Notes: - We now always fetch the docker binary on use, instead of checking for its existence locally. This avoids some code complexity and makes sure we always use the expected version. The downside should be mitigated by the pre-fetching in built containers, and the fetch-tools command. - We really need to reduce the number of tools and versions we bundle before we release 0.12. The container image sizes are too large atm.

edvald assigned eysi09 Jun 4, 2020

edvald force-pushed the tools-command branch from cc52deb to 87ed678 Compare June 4, 2020 11:47

edvald added this to the 0.12.0 milestone Jun 4, 2020

edvald force-pushed the tools-command branch from 87ed678 to 392b59c Compare June 4, 2020 21:30

eysi09 reviewed Jun 9, 2020

View reviewed changes

edvald added 2 commits June 9, 2020 20:01

fix: review comments (TBS)

58656a9

edvald force-pushed the tools-command branch from 392b59c to 58656a9 Compare June 9, 2020 18:10

eysi09 approved these changes Jun 9, 2020

View reviewed changes

edvald merged commit 65e3251 into 0.12 Jun 9, 2020

edvald deleted the tools-command branch June 9, 2020 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): pre-fetch provider tools and make tools a native feature #1858

feat(core): pre-fetch provider tools and make tools a native feature #1858

edvald commented Jun 4, 2020

eysi09 left a comment

eysi09 Jun 9, 2020

edvald Jun 9, 2020

eysi09 Jun 9, 2020

edvald Jun 9, 2020

eysi09 Jun 9, 2020

edvald Jun 9, 2020

edvald Jun 9, 2020

eysi09 Jun 9, 2020

edvald Jun 9, 2020

eysi09 Jun 9, 2020

eysi09 Jun 9, 2020

edvald Jun 9, 2020

eysi09 Jun 9, 2020

edvald Jun 9, 2020

edvald commented Jun 9, 2020

eysi09 commented Jun 9, 2020

edvald commented Jun 9, 2020

feat(core): pre-fetch provider tools and make tools a native feature #1858

feat(core): pre-fetch provider tools and make tools a native feature #1858

Conversation

edvald commented Jun 4, 2020

eysi09 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edvald commented Jun 9, 2020

eysi09 commented Jun 9, 2020

edvald commented Jun 9, 2020