Support Google Container Registry #26

petemounce · 2020-05-22T09:05:35Z

Not sure whether this will be a welcome addition (no reply inside #24), but I decided to make a start since I'll be able to use this via our fork either way.

What's going on here:

Extracted the ECR-specific functions to the ecr-registry-provider.bash include.
- They're verbatim implementations, but a couple had their names changed since there'll be GCR-specific implementations so I made the names generalised.
Introduced the provider-specific include at the top of hooks/pre-command. This will fail with an error if the plugin is misconfigured.

Initial commit is just this. I'm still interested to see whether the maintainers (I assume @zsims ?) are interested in this addition.

Still to-do (whether that's this PR or followups)

docs updates
GCR implementation
- BATS for GCR implementation

…fore max-age (those are done via GCS TTL policy)

zsims · 2020-05-25T23:26:23Z

Hey, thanks for the PR! Sorry for no reply on #24 -- has been a bit crazy :S

I think this does make sense in the same plugin. I was originally thinking it would be easier to have a separate plugin, but with the same interface. However, one thing to keep in mind would be testing -- we don't use GCR so we'd have to rely on the bats tests.

zsims

Looks great, would be happy to accept this with the tests. I reckon more overlap/same interface the better

hooks/lib/ecr-registry-provider.bash

hooks/lib/gcr-registry-provider.bash

hooks/lib/stdlib.bash

Co-authored-by: Zac Sims <zsims@users.noreply.github.com>

…rd dockerfile location, OR file path separators...

petemounce · 2020-06-01T10:26:05Z

(This commit stream is me iterating a docker build on a Windows machine, and I'm having a great time, clearly)

zsims · 2020-06-04T08:18:26Z

hooks/pre-command

  echoerr '--- Computing tag'

  echoerr 'DOCKERFILE'
-  echoerr "+ ${docker_file}:${target:-<target>}"
+  echoerr "+ ${docker_file}:${target:-"<target>"}"


Are these extra quotes required?

No, that's muscle memory on my part. I tend to always enclose strings in quotes and variables inside {} because it's easier than remembering where it will be necessary and where it won't be necessary, compared to just reflexively doing it everywhere.

petemounce · 2020-06-15T19:09:42Z

@zsims please take another look? I've added test coverage, and intend to add 2 more to pre-command.bats.

petemounce · 2020-06-15T19:38:36Z

@zsims Please also see #27?

I reckon this is now ready for re-review. Since last time, I have

moved the functions from hooks/pre-command to hooks/lib/stdlib.bash to move them out the way. At the time, I'd hoped it was possible to stub out bash functions, but I later didn't find out how, so ...
I added hooks/lib/stub-registry-provider.bash which makes stubs for things so that the tests/pre-command.bats didn't have to repeat...
... the test coverage I wrote in tests/stdlib.bash
added test coverage tests/gcr-registry-provider.bats

That takes us from 2 tests to 17. I never knew it was so comparatively straightforward to write bash test coverage, so I'm really grateful for the lesson!

In terms of backwards compatibility, is there anything you'd like me to do (though, probably if so, in a follow-up - this PR has ballooned!). Things I thought of:

make additional-build-args an array instead of a string, since that simplifies that bit of script that glues that onto the end of the docker build down to "${BUILDKITE_PLUGIN_DOCKER_ECR_CACHE_ADDITIONAL_BUILD_ARGS[@]}"
add .pre-commit-config.yaml with shellcheck and shfmt and some other quality-of-life stuff (see pre-commit.com)
make the target be an arg to compute_tag instead of leaking in from outer scope
Add a plugin property image-name to replace ecr-name
- ... but maintain backwards compatibility by coalescing back to ecr-name's env var
Maybe talk about renaming the repository to docker-cache-buildkite-plugin?
- That has some entertaining injection security implications, however - github will redirect requests both UI and clone to the new name from the old name. However, if someone makes a new replacement docker-ecr-cache-buildkite-plugin with the same tags as here, that will silently man-in-the-middle peoples' pipelines.
- perhaps @lox or @toolmantim might like to give some advice on what is best to do here?

petemounce · 2020-06-15T21:50:46Z

(The tests run and pass on Windows and macOS hosts)

toolmantim · 2020-06-15T22:44:59Z

Sounds like a rename is a good idea!

If you trust seek-oss enough to not use SHA-pinned plugins, then perhaps it's probably okay to let the name sit there available for another repo to steal back at some point?

petemounce · 2020-06-16T10:38:28Z

I'm a very trusting sort. I'd actually forgotten that it's an commercial org :)

toolmantim · 2020-06-16T10:52:27Z

😁

72636c · 2020-06-18T16:17:02Z

Hey, thanks for this @petemounce! @zsims has subbed out but I will try to review soon 🙏

RE: the rename, I think the least surprising way would be to fork this repo then archive the current one.

72636c

Thanks for your patience! It's been pretty full-on here over the past few weeks. This is a massive improvement and the tests are awesome 🙏

Just have a few nits that should be easily resolved. Let me know if you're busy and I can run with it.

hooks/lib/stdlib.bash

hooks/pre-command

72636c · 2020-06-26T02:04:21Z

hooks/lib/gcr-registry-provider.bash

+    echoerr "registry-hostname had no value, defaulting to gcr.io"
+    BUILDKITE_PLUGIN_DOCKER_ECR_CACHE_REGISTRY_HOSTNAME="gcr.io"
+  fi
+  echo "${BUILDKITE_PLUGIN_DOCKER_ECR_CACHE_REGISTRY_HOSTNAME}/${BUILDKITE_PLUGIN_DOCKER_ECR_CACHE_GCP_PROJECT}/${BUILDKITE_PLUGIN_DOCKER_ECR_CACHE_ECR_NAME:-"$(get_default_image_name)"}"


Ugh. Not for this PR, but we should definitely have a generic alias for this option as you've suggested.

Note that the README says that this is an ECR-specific option, so we should either update it or drop this logic from here until we have that image-name.

Let me know which way you'd prefer to go on this one ^ @petemounce

Apart from that, looks all ready to merge!

I think I'd prefer to leave this as-is within this PR, and iterate afterwards.

Actually, wait - I'm not sure I follow what you're suggesting. Please would you elaborate?

GCR URIs take the form https://{host}/{gcp-project}/{container}:{tag} - so, actually, I think container` is the equivalent to ECR's repository now that I think about this more freshly.

I think container is the equivalent to ECR's repository

Yep, looks that way to me too. Happy to iterate on it.

Above I was thinking that we should document that ecr-name affects the GCR configuration, but it's probably not necessary given it's optional and we'd probably want to fix up the name in future.

hooks/pre-command

Co-authored-by: Ryan Ling <ryan@outlook.com.au>

72636c · 2020-06-28T21:48:46Z

hooks/lib/gcr-registry-provider.bash

+    echoerr "registry-hostname had no value, defaulting to gcr.io"
+    BUILDKITE_PLUGIN_DOCKER_ECR_CACHE_REGISTRY_HOSTNAME="gcr.io"
+  fi
+  echo "${BUILDKITE_PLUGIN_DOCKER_ECR_CACHE_REGISTRY_HOSTNAME}/${BUILDKITE_PLUGIN_DOCKER_ECR_CACHE_GCP_PROJECT}/${BUILDKITE_PLUGIN_DOCKER_ECR_CACHE_ECR_NAME:-"$(get_default_image_name)"}"


Let me know which way you'd prefer to go on this one ^ @petemounce

Apart from that, looks all ready to merge!

Peter Mounce added 4 commits May 22, 2020 09:58

Support GCR step 1; factor to library

1c532e0

more refactor - GCR has no concept of repository for images nor there…

8c7a037

…fore max-age (those are done via GCS TTL policy)

tweaks

b7eac51

GCR WIP

c1da4c6

zsims reviewed May 25, 2020

View reviewed changes

hooks/lib/ecr-registry-provider.bash Show resolved Hide resolved

hooks/lib/gcr-registry-provider.bash Show resolved Hide resolved

hooks/lib/stdlib.bash Outdated Show resolved Hide resolved

Peter Mounce and others added 16 commits May 29, 2020 14:58

Update hooks/lib/stdlib.bash

8efbff2

Co-authored-by: Zac Sims <zsims@users.noreply.github.com>

debug issue on windows, suspect missing context value with non-standa…

de89d9a

…rd dockerfile location, OR file path separators...

fail-faster with error message

d82ea7e

More debug

222a7e2

refactor, show where cwd is

d27e6c6

doh

451a9b9

argh

5d45320

weird?

850ce86

wat.

a99e461

uh

86a280a

really?

c5b1abd

file

74f6729

quotes

1558ce0

'

591d4dd

really

085b013

sigh

82d1bdd

Peter Mounce added 3 commits June 1, 2020 18:49

works internally. It's always quoting, always....

b2a7216

docs, plugin definition, more refinement

7fd1fd2

oops, err

00f3abb

zsims reviewed Jun 4, 2020

View reviewed changes

petemounce mentioned this pull request Jun 15, 2020

Please support BATS 1.2.0? buildkite-plugins/buildkite-plugin-tester#28

Closed

petemounce added 2 commits June 15, 2020 17:15

Added BATS coverage for stdlib and GCR-specific function implementations

a741c3e

logic coverage 1

5e59559

Remainder of coverage

c92e1c9

72636c mentioned this pull request Jun 25, 2020

Allow custom context support. #28

Merged

72636c reviewed Jun 26, 2020

View reviewed changes

better regex match choice

4f78c96

Co-authored-by: Ryan Ling <ryan@outlook.com.au>

72636c approved these changes Jun 28, 2020

View reviewed changes

72636c merged commit fb591eb into seek-oss:master Jul 9, 2020

petemounce deleted the support-gcr-1 branch July 9, 2020 17:10

petemounce restored the support-gcr-1 branch July 9, 2020 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Google Container Registry #26

Support Google Container Registry #26

petemounce commented May 22, 2020 •

edited

zsims commented May 25, 2020

zsims left a comment

petemounce commented Jun 1, 2020

zsims Jun 4, 2020

petemounce Jun 4, 2020

petemounce commented Jun 15, 2020

petemounce commented Jun 15, 2020 •

edited

petemounce commented Jun 15, 2020

toolmantim commented Jun 15, 2020

petemounce commented Jun 16, 2020

toolmantim commented Jun 16, 2020

72636c commented Jun 18, 2020

72636c left a comment

72636c Jun 26, 2020

72636c Jun 28, 2020

petemounce Jul 7, 2020

petemounce Jul 7, 2020

72636c Jul 9, 2020 •

edited

72636c Jun 28, 2020

Support Google Container Registry #26

Support Google Container Registry #26

Conversation

petemounce commented May 22, 2020 • edited

What's going on here:

Still to-do (whether that's this PR or followups)

zsims commented May 25, 2020

zsims left a comment

Choose a reason for hiding this comment

petemounce commented Jun 1, 2020

zsims Jun 4, 2020

Choose a reason for hiding this comment

petemounce Jun 4, 2020

Choose a reason for hiding this comment

petemounce commented Jun 15, 2020

petemounce commented Jun 15, 2020 • edited

petemounce commented Jun 15, 2020

toolmantim commented Jun 15, 2020

petemounce commented Jun 16, 2020

toolmantim commented Jun 16, 2020

72636c commented Jun 18, 2020

72636c left a comment

Choose a reason for hiding this comment

72636c Jun 26, 2020

Choose a reason for hiding this comment

72636c Jun 28, 2020

Choose a reason for hiding this comment

petemounce Jul 7, 2020

Choose a reason for hiding this comment

petemounce Jul 7, 2020

Choose a reason for hiding this comment

72636c Jul 9, 2020 • edited

Choose a reason for hiding this comment

72636c Jun 28, 2020

Choose a reason for hiding this comment

petemounce commented May 22, 2020 •

edited

petemounce commented Jun 15, 2020 •

edited

72636c Jul 9, 2020 •

edited