go module type preliminary/prototype #1271

vkorbes · 2019-10-17T14:35:04Z

This is very preliminary work. Do not merge.

I'll fill out this PR tomorrow.

@edvald I'm currently studying garden-service/src/types/plugin/service/deployService.ts and garden-service/src/plugins/kubernetes/container/deployment.ts to try and figure out how to do the hot reload part. Any direction there is welcome. (Thanks!)

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

eysi09 · 2019-10-17T14:49:21Z

@ellenkorbes FYI: You can flag PRs as drafts if you don't intend to merge them as is.

edvald · 2019-10-17T19:02:07Z

garden-service/src/plugins/go-container/go.ts

+  specs: {
+    darwin: macos,
+    linux: penguin,
+    // Is the "32" here really 32 or is it just the name?


It's just a relic, standard Node.js platform names that we match.

edvald · 2019-10-17T19:03:20Z

garden-service/src/plugins/go-container/go-container.ts

+  }
+
+  // What was this for?
+  // await copy(resolvedJarPath, resolve(module.buildPath, "app.jar"))


This was needed in the maven-container module type, because the Jar file there is generated inside the user's source folder.

edvald · 2019-10-17T19:04:44Z

garden-service/src/plugins/go-container/go-container.ts

+
+  // If they did, update the vendor directory
+  console.log(`Downloading new dependencies...`)
+  if (tidy.stdout != "") {


We should always use !==, but you can also just use if (tidy.stdout) here.

edvald · 2019-10-17T19:06:46Z

garden-service/src/plugins/go-container/go-container.ts

+    try {
+    await ensureDir(resolve(module.path, "bin"))
+  } catch (err) {
+    console.error(err)


I don't believe ensureDir() should ever error, no need for the try/catch.

It'll error if it tries to create a directory one doesn't have the permissions to create (e.g. https://runkit.com/57ebc041d4a27f140028fd78/5a33f49ef88d280012b07780) But you're right, this it too much of an edge case.

edvald · 2019-10-17T19:08:37Z

garden-service/src/plugins/go-container/go-container.ts

+    .example("false"),
+  // compress: joi.alternatives().try(
+  //     joi.boolean(),
+  //     joi.number(),


You can further specify the allowed numeric values: joi.number().integer().min(1).max(9)

Actually, I think we should omit this option entirely. It adds unnecessary complexity. Compression is anyway applied in container images, and when syncing during hot-reloading.

I'll leave it there commented out until the whole thing is fully functional and I can benchmark it. I suspect it could make a significant difference for large binaries.

Might be worth trying as a stretch goal, but I'd be quite surprised if it made a meaningful difference, since data is anyway compressed when going over the wire.

edvald · 2019-10-17T19:09:43Z

garden-service/src/plugins/go-container/go-container.ts

+  //     joi.boolean(),
+  //     joi.number(),
+  //   ).description(dedent`
+  //   "By default, Garden will compress the resulting Go binary with UPX \(upx.github.io\).


Extra quote-mark at the start. And the escaping is not needed on the parentheses.

edvald · 2019-10-17T19:11:19Z

garden-service/src/plugins/go-container/go-container.ts

+  //   For this functionality to work with macOS, UPX needs
+  //   to first be installed with Homebrew using \`brew install upx\`.
+  //   `)
+  //   .example("1"),


The example should be an actual value, e.g. 1 and not "1", since the example value is actually validated when the schema is defined. You can also set the default here (.default(9)).

edvald · 2019-10-17T19:12:21Z

garden-service/src/plugins/go-container/go-container.ts

+  })
+} catch (err) {
+  console.log(err.stderr)
+}


I see these spacing issues here and there. I highly recommend installing the TSLint plugin in VSCode, which will automatically correct these.

edvald · 2019-10-17T19:14:59Z

garden-service/src/plugins/go-container/go-container.ts

+  log.setState(`Starting Go build process...`)
+
+  // Check if any imports have changed
+  console.log(`Checking for new dependencies...`)


I recommend adding a status subline for these logs:

const status = log.info("Checking for new dependencies...") // And then for the next status message below ... status.setState("Downloading new dependencies...")

edvald · 2019-10-17T19:18:14Z

garden-service/src/plugins/go-container/go-container.ts

+      By default, Garden passes the '-w' flag to the linker to omit this information and make the resulting binaries smaller.
+      If you do want this information, set this option to \`true\`.
+    `)
+    .example("false"),


Change to .example(true) (no quotes)
And add .default(false)

edvald · 2019-10-17T19:37:02Z

garden-service/static/go-container/go-container.Dockerfile

+RUN apk add entr ca-certificates
+RUN mkdir app
+WORKDIR /app
+EXPOSE 8080


The EXPOSE directive is not necessary, easier to just omit it.

edvald · 2019-10-17T19:37:44Z

garden-service/static/go-container/go-container.Dockerfile

@@ -0,0 +1,9 @@
+FROM alpine


We should definitely specify an Alpine version (just go with whatever is most recent).

edvald · 2019-10-17T19:40:03Z

garden-service/static/go-container/go-container.Dockerfile

+
+COPY bin/ /app/
+
+CMD ls binary | entr -r ./binary 


I realize this is more of a placeholder, but we should use ENTRYPOINT instead of CMD here and make it simply ENTRYPOINT [/app/bin]. When we use hot reloading, we'll anyway override the command in the manifest.

edvald · 2019-10-17T19:42:05Z

garden-service/src/plugins/go-container/go-container.ts

+      directory.
+
+      To use it, make sure to add the \`maven-container\` provider to your project configuration.
+      The provider will automatically fetch and cache Maven and the appropriate OpenJDK version ahead of building.


Let's remember to update this text, and also to add the module type to our reference docs.

edvald · 2019-10-17T19:47:25Z

garden-service/src/plugins/go-container/go-container.ts

+    env: {
+      CGO_ENABLED: "0",
+      GOOS: "linux",
+      GOCACHE: resolve(module.buildPath, "cache"),


Why do we override the cache?

Go's default cache location is ~/.cache/. Since we're keeping the Go install compartmentalized inside the module, I figured we should do the same for the compiler cache.

Which raises an interesting question. If a user has 10 Go projects, they'll have 10 copies of Go in their machine. Should we keep this in a central location e.g. ~/.garden/go?

We download Go once for each user. So that won't be an issue.

And I don't really see a reason to partition the cache by module. If different modules vendor the same libraries it's just less efficient to cache separately. Unless there's some concern specific to Go that I'm not aware of?

That refers to the compiler cache, for .a files and such.

Libraries are vendored separately by default. We could come up with a hack to have a central location for all of them, but that'd cause version conflicts e.g. when one project uses version v1 of a library and the other one uses v2.

I'm not suggesting doing anything unusual, just to leave the default GOCACHE setting. The default is "central" anyway, in that there's a shared cache directory. Unless there are known issues with the default (which would be pretty odd) I still don't see a reason to override it.

edvald · 2019-10-17T19:50:52Z

garden-service/src/plugins/go-container/go-container.ts

+  debug: boolean
+  compress: boolean | number
+  run: string
+  buildFlags: string


Both run and buildFlags should be string[]

edvald · 2019-10-17T19:51:27Z

garden-service/src/plugins/go-container/go-container.ts

+  // await prepareBuild(module, log)
+
+  return base!(params)
+}


I think we can remove this handler.

edvald · 2019-10-17T20:02:40Z

garden-service/src/plugins/go-container/go-container.ts

+//   } catch (err) {
+//     console.log(err)
+//   }
+// }


This is quite messy. Better to use the buildArgs field on the container module type. See the maven-container module type configure handler for how to do it:

garden/garden-service/src/plugins/maven-container/maven-container.ts

Line 129 in fca6a72

containerConfig.spec.buildArgs = {

You'd do something like

containerConfig.spec.buildArgs = { EXTRA_RUN: moduleConfig.spec.build.run.join(" ") || "echo" }

And then add RUN ${EXTRA_RUN} at the bottom of the Dockerfile.

edvald · 2019-10-17T20:26:33Z

Re: Adding hot-reloading, it should actually be reasonably simple. First override the hotReloadCommand for each service in the configure handler, and auto-populate the hotReload.sync field. Something like (I've not tested the code btw, just illustrating the idea):

containerConfig.spec.hotReload = containerConfig.spec.hotReload || {
  sync: [{ source: "./bin", target: "/app/bin" }],
}

for (const service of containerConfig.spec.services || []) {
  service.hotReloadCommand = service.hotReloadCommand || [<the entr base command>]
}

Then you need to ensure the binary is built before the hot reloading happens:

async function hotReload(params: HotReloadServiceParams<GoContainerModule>) {
  const { base, module, log } = params

  // Need to extract the binary build from the build handler into a separate function
  await buildBinary(module, log)

  // Defer the rest of it to the kubernetes provider (or whatever other provider applies, technically)
  return base!(params)
}

The kubernetes provider will then take care of the rest.

eysi09 · 2019-10-22T20:02:27Z

garden-service/src/plugins/go-container/go-container.ts

+}
+
+const goContainerModuleSpecSchema = containerModuleSpecSchema.keys({
+  build: baseBuildSpecSchema.keys(goKeys),


The module spec should be top level, not under the build key. So basically: const goContainerModuleSpecSchema = containerModuleSpecSchema.keys(goKeys). Fixes the validation error we were seeing.

This was intentional, since all those parameters have to do with the build. It's admittedly clumsy to handle the schema this way, but we'll deal with that later at the framework level.

edvald · 2019-10-31T18:03:49Z

You can now rebase this branch off master, since the branch it depended on has been merged. Will also make this easier to review and iterate on.

eysi09 · 2020-01-14T20:12:18Z

Closing in favour of #1511.

vkorbes added the feature label Oct 17, 2019

vkorbes requested a review from edvald October 17, 2019 14:35

edvald reviewed Oct 17, 2019

View reviewed changes

eysi09 reviewed Oct 22, 2019

View reviewed changes

edvald force-pushed the module-type-inheritance branch 5 times, most recently from 98209ff to ed4eac3 Compare October 30, 2019 14:42

edvald force-pushed the go branch from 6cb54f9 to 7bbe5cd Compare November 12, 2019 22:28

vkorbes added 2 commits November 12, 2019 23:49

go module type preliminary/prototype

7576505

hot reload hates me

c129d57

edvald force-pushed the go branch from 7bbe5cd to c129d57 Compare November 12, 2019 22:50

edvald changed the base branch from module-type-inheritance to master November 12, 2019 22:50

eysi09 mentioned this pull request Jan 14, 2020

Add a dedicated Go module type #1511

Closed

eysi09 closed this Jan 14, 2020

thsig deleted the go branch May 9, 2022 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go module type preliminary/prototype #1271

go module type preliminary/prototype #1271

vkorbes commented Oct 17, 2019

eysi09 commented Oct 17, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

vkorbes Oct 18, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

vkorbes Oct 18, 2019

edvald Oct 18, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

vkorbes Oct 18, 2019

edvald Oct 18, 2019

vkorbes Oct 18, 2019

edvald Oct 18, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

edvald Oct 17, 2019

edvald commented Oct 17, 2019

eysi09 Oct 22, 2019

edvald Oct 22, 2019

edvald commented Oct 31, 2019

eysi09 commented Jan 14, 2020

go module type preliminary/prototype #1271

go module type preliminary/prototype #1271

Conversation

vkorbes commented Oct 17, 2019

eysi09 commented Oct 17, 2019

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

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 Oct 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edvald commented Oct 31, 2019

eysi09 commented Jan 14, 2020