refactor build tooling #114

montmanu · 2019-07-18T15:44:37Z

Summary

Adds required GCLOUD_SDK_TAG --build-arg to Dockerfile.

Removes Dockerfile.1.10, Dockerfile.1.11; the cloud-sdk version (and by proxy, the kubectl version) can / must now be customized via the new GCLOUD_SDK_VERSION build arg.

Removes bin/dist; replaced with make-based implementation.

Adds several misc config / rc files (e.g., .dockerignore, .editorconfig, etc).

Updates README, DOCS.md to reflect changes, address lint issues.

Motivation

This changeset evolved from the changes included in #115 and the primary motivation was to enable a tighter local development cycle / shorter iterations. These changes were separated from #115 primarily to simplify code review.

With these two changes, we may be able to address improvements / feature requests (e.g., #29 , #104 , #105 , etc) with less friction, barriers to entry for new contributors, more flexibility, etc.

- enables running the plugin using a non-default `kubectl` executable - updates image `ENTRYPOINT` to support setting of `EXTRA_KUBECTL_VERSIONS` environment variable used by plugin to validate against `kubectl_version` depends on #114

fsouza

Looks good! Just one question about the Dockerfile.

Dockerfile

montmanu

some notes to self

README.md

Makefile

tonglil

Thanks for refactoring the build process.

Can you move the usage information above the develop information?

I like the changes to the pipeline, but I have concerns about the makefile:

it adds maintenance complexity
it's probably better to keep the publishing task to the ci/cd pipeline, vs documenting the publishing flow through the make file - now there are 2 ways to accomplish the same thing
see other comment

Dockerfile

Makefile

tonglil · 2019-07-19T20:52:38Z

Makefile

+usage :
+	@echo "Usage:"
+	@echo ""
+	@echo "  \$$ make check               # run tests for required software"


is checking necessary - wouldn't the commands fail if it's not installed anyways, or does make hide the error message?

the commands would fail, but the thinking behind these check-* targets is that each could be added as a prerequisite to another target .. effectively becoming a hook to provide some feedback to a user about installing whatever dependency they're missing:

# compile binary $(binary_name) : check-go # ...

if the user doesn't have go installed, we could include something like a link to install documentation

that said, not at all opposed to removing these.. is that what you'd prefer?

Since it's written, let's keep it.
I think if it becomes a cause of errors then we can remove it then to reduce points of maintenance :)

README.md

- adds required `GCLOUD_SDK_VERSION` `--build-arg` to `Dockerfile` - removes `Dockerfile.1.10`, `Dockerfile.1.11` (can now be customized via `GCLOUD_SDK_VERSION` build arg) - removes `bin/dist`; replaced with `make`-based implementation - adds several misc config / rc files - updates `README`, `DOCS.md` to reflect changes, address linting refactor build tooling - adds required `GCLOUD_SDK_VERSION` `--build-arg` to `Dockerfile` - removes `Dockerfile.1.10`, `Dockerfile.1.11` (can now be customized via `GCLOUD_SDK_VERSION` build arg) - removes `bin/dist`; replaced with `make`-based implementation - adds several misc config / rc files - updates `README`, `DOCS.md` to reflect changes, address linting Apply suggestions from code review Co-Authored-By: Chris Montoro <[email protected]>

- remove cloud-sdk stage from Dockerfile - remove bin/install-kubectl ; add installation steps inline within Dockerfile - replaced references to CLOUD_SDK_VERSION / GCLOUD_SDK_VERSION with GCLOUD_SDK_TAG - move developer focused documentation to CONTRIBUTING.md - move .PHONY blocks to the line above each related target definition - add coverage.out make target - add check for go v 1.12+ - rename docker-release, docker-run-local make targets - add terraform integration to automate test resource provisioning - update DOCS.md to support deep links to individual parameters within API reference - update local-example app, image, API_TOKEN variable names to avoid potential confusion with drone-gke parameters

- enables running the plugin using a non-default `kubectl` executable - updates image `ENTRYPOINT` to support setting of `EXTRA_KUBECTL_VERSIONS` environment variable used by plugin to validate against `kubectl_version` depends on #114

tonglil · 2019-07-25T20:43:09Z

perhaps some of the make-related documentation here would be better off in CONTRIBUTING.md?

Yes.

tonglil · 2019-07-25T20:47:15Z

DOCS.md

-* `vars` - variables to use in `template` and `secret_template` (see [below](#available-vars) for details)
-* `secrets` - credential and variables to use in `secret_template` (see [below](#secrets) for details)
-* *optional* `expand_env_vars` - expand environment variables for values in `vars` for reference (defaults to `false`)
+### `image`


This is very verbose, I'd like to stick to the original format, or in the drone plugins registry format:

http://plugins.drone.io/drone-plugins/drone-docker/

https://github.com/jmccann/drone-terraform/blob/master/DOCS.md

is there any automation built around those formats? happy to use a less verbose alternative, just thinking it would be helpful to enable deep linking directly to a specific parameter's documentation..

AFAIK no there isn't.

hm if there isn't any automation built around the previous format or the other formats used, is there a functional downside to these changes (aside from the more verbose markdown source)?

in addition to the deep links, i think some folks might find the in-context examples helpful.. for a concrete case, i myself had to view the source of the plugin to find out how to use the wait_deployments option (do i provide a string? if so, separated by what character? there aren't any other options that accept an array of values so it wasn't clear that this one was expected to be provided as such) .. for another, i wasn't aware that values within vars could be any arbitrary value (e.g., could include more than the primitive string / int types).. that might have been more obvious if the type info was included in the docs for each option

tonglil · 2019-07-25T20:47:28Z

DOCS.md

@@ -68,7 +417,7 @@ If connecting to a [zonal cluster](https://cloud.google.com/kubernetes-engine/do

 The `zone` and `region` parameters are mutually exclusive; providing both to the plugin for the same execution will result in an error.

-## Secrets
+## Using `secrets`


tonglil · 2019-07-25T20:47:34Z

DOCS.md

-	"namespace": "",
-	"project": "my-gcp-proj",
-	"zone": "us-east1-a"
+  "BRANCH": "master",


tonglil · 2019-07-25T20:51:11Z

README.md


 Executing locally from the working directory:

-```
+```sh


tonglil · 2019-07-25T20:52:34Z

local-example/.kube.yml


 spec:
  replicas: 3
  template:
    metadata:
      labels:
-        app: {{.app}}
+        app: {{.app_name}}


can we keep this as {{.app}}?

tonglil · 2019-07-25T20:52:41Z

local-example/.kube.yml

        env: {{.env}}
    spec:
      containers:
        - name: app
-          image: {{.image}}
+          image: {{.app_image}}


and this as {{.image}}

so while working with the project and documenting a few commands / parameters, i noticed this (as well as the {{.app}} and {{.API_TOKEN}} references) were potentially confusing

specifically, it seemed like folks might be easily confused between{{.API_TOKEN}} and secrets: { source: GOOGLE_CREDENTIALS, target: token } especially when looking for token and seeing references to both the SA credential use case and the secretRef use case.. same with the k8s container spec image use case and the drone step image use case, as well as in the k8s container spec name use and the pod spec label use case..

more generally, the idea here was to try and distinguish between references related to a user's use case and references related to the plugin's use case..

mind elaborating a bit on your concerns here?

oliver-nyt · 2019-07-25T19:16:37Z

.editorconfig

@@ -0,0 +1,21 @@
+root = true


Sure you want to check this in?

@oliver-nyt happy to remove, but i think projects tend to set this to prevent configurations higher up in the filesystem tree (e.g, user settings) from resulting in auto generated changes that may not be desired within the project.. here are a few selected examples from the projects listed here:

htop
qemu
SpringBoot

wdyt? keep or remove?

Your call. I personally don't like checking in user tools specific stuff like editor configs (this is for VSCode only, right?) but I see your point, it makes life of some users easier. Interesting that so many projects do it.

editorconfig isn't specific to vscode: https://editorconfig.org/#download 😁

CONTRIBUTING.md

fsouza

I second @oliver-nyt's comment on Go version, LGTM otherwise 🚀

fsouza

One question about check-go, because if failed locally 😬

Makefile

montmanu · 2019-07-29T20:12:18Z

@oliver-nyt @fsouza re: the version of go there, i was basing that off of

drone-gke/go.mod

Line 3 in 483e946

go 1.12

happy to remove the golang version checks if thats going to be a pain point

- enables running the plugin using a non-default `kubectl` executable - updates image `ENTRYPOINT` to support setting of `EXTRA_KUBECTL_VERSIONS` environment variable used by plugin to validate against `kubectl_version` depends on #114

montmanu · 2019-07-31T01:56:24Z

@tonglil @fsouza @oliver-nyt any other feedback / requested changes here, or is everyone comfortable merging at this point?

oliver-nyt · 2019-07-31T03:02:30Z

Go for it

fsouza · 2019-07-31T04:53:46Z

good to go!

- enables running the plugin using a non-default `kubectl` executable - updates image `ENTRYPOINT` to support setting of `EXTRA_KUBECTL_VERSIONS` environment variable used by plugin to validate against `kubectl_version` depends on #114

* add new `kubectl_version` param - enables running the plugin using a non-default `kubectl` executable - updates image `ENTRYPOINT` to support setting of `EXTRA_KUBECTL_VERSIONS` environment variable used by plugin to validate against `kubectl_version` depends on #114 * update DOCS * update validation implementation, docs; add new option to make run target * rename entrypoint to set-env-versions

montmanu requested review from tonglil and fsouza July 18, 2019 15:45

montmanu mentioned this pull request Jul 18, 2019

add new kubectl_version param #115

Merged

fsouza approved these changes Jul 19, 2019

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

montmanu commented Jul 19, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

tonglil mentioned this pull request Jul 19, 2019

Support 1.12, remove 1.10 #112

Closed

tonglil reviewed Jul 19, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

montmanu added 2 commits July 21, 2019 21:56

montmanu force-pushed the refactor-build branch from 07c0013 to bc8b0a1 Compare July 22, 2019 01:57

montmanu requested a review from tonglil July 22, 2019 02:06

montmanu added 2 commits July 21, 2019 22:33

add missing alpine variant to GCLOUD_SDK_TAG for 1.10, 1.11 versions

b3a9980

fix terraform executable name, anchor links

515d7c6

fix terraform provider configuration

483e946

tonglil reviewed Jul 25, 2019

View reviewed changes

oliver-nyt reviewed Jul 26, 2019

View reviewed changes

fsouza reviewed Jul 26, 2019

View reviewed changes

fsouza reviewed Jul 29, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

remove version checks from check-go, check-terraform

af81393

remove note regarding golang version requirements

5fc3fb9

montmanu merged commit 6c66920 into master Jul 31, 2019

montmanu deleted the refactor-build branch July 31, 2019 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor build tooling #114

refactor build tooling #114

montmanu commented Jul 18, 2019 •

edited

Loading

fsouza left a comment

montmanu left a comment

tonglil left a comment

tonglil Jul 19, 2019

montmanu Jul 20, 2019 •

edited

Loading

tonglil Jul 25, 2019

This comment has been minimized.

tonglil commented Jul 25, 2019

tonglil Jul 25, 2019

montmanu Jul 25, 2019

tonglil Jul 25, 2019

montmanu Jul 29, 2019

tonglil Jul 25, 2019

tonglil Jul 25, 2019

tonglil Jul 25, 2019

tonglil Jul 25, 2019

tonglil Jul 25, 2019

montmanu Jul 25, 2019

oliver-nyt Jul 25, 2019

montmanu Jul 29, 2019

oliver-nyt Jul 30, 2019

fsouza Jul 30, 2019

oliver-nyt Jul 30, 2019

fsouza left a comment

fsouza left a comment

montmanu commented Jul 29, 2019

montmanu commented Jul 31, 2019

oliver-nyt commented Jul 31, 2019

fsouza commented Jul 31, 2019

refactor build tooling #114

refactor build tooling #114

Conversation

montmanu commented Jul 18, 2019 • edited Loading

Summary

Motivation

fsouza left a comment

Choose a reason for hiding this comment

montmanu left a comment

Choose a reason for hiding this comment

tonglil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montmanu Jul 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

tonglil commented Jul 25, 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

fsouza left a comment

Choose a reason for hiding this comment

fsouza left a comment

Choose a reason for hiding this comment

montmanu commented Jul 29, 2019

montmanu commented Jul 31, 2019

oliver-nyt commented Jul 31, 2019

fsouza commented Jul 31, 2019

montmanu commented Jul 18, 2019 •

edited

Loading

montmanu Jul 20, 2019 •

edited

Loading