Page MenuHomePhabricator

Unify configuration for local build-context copies and variant artifacts
Closed, ResolvedPublic

Description

Original title was "Manually defining artifacts results in default copy of all project files"

Correct case

Using the copies: [variantref] pattern in a variant works as expected: Instead of the application and support files being copied in using COPY . ., a set of multi-stage artifacts are defined using COPY --from=[variantref].

version: v3
base: docker-registry.wikimedia.org/wikimedia-stretch:latest
lives:
  in: /go/src/gerrit.wikimedia.org/r/blubber

variants:
  build:
    base: golang:1.9-stretch
  test:
    includes: [build]
    runs: { insecurely: true }
    builder:
      command: [go, get, -u, golang.org/x/lint/golint]
    entrypoint: [make, test]
  prep:
    includes: [build]
    builder:
      command: [make, blubberoid]
      requirements: [.]
  production:
    copies: prep
    entrypoint: [./blubberoid]

Resulting Dockerfile (irrelevant lines omited):

FROM golang:1.9-stretch AS prep
WORKDIR "/go/src/gerrit.wikimedia.org/r/blubber"
COPY --chown=65533:65533 [".", "."]

FROM docker-registry.wikimedia.org/wikimedia-stretch:latest AS production
COPY --chown=65533:65533 --from=prep ["/go/src/gerrit.wikimedia.org/r/blubber", "/go/src/gerrit.wikimedia.org/r/blubber"]
COPY --chown=65533:65533 --from=prep ["/opt/lib", "/opt/lib"]

Errant case

However, in a use case where you only want a subset of artifacts (e.g. for Blubberoid we only need one static binary), manually specifying that subset of artifacts results in unexpected behavior: All application files are now copied over as well and there's no current way to omit them.

(irrelevant lines from previous example omitted)

variants:
  production:
    lives: { in: /srv/service }
    artifacts:
    - from: prep
      source: "/go/src/gerrit.wikimedia.org/r/blubber/blubberoid"
      destination: "./"
    entrypoint: [./blubberoid]

Resulting Dockerfile (irrelevant lines omitted):

FROM golang:1.9-stretch AS prep
WORKDIR "/go/src/gerrit.wikimedia.org/r/blubber"
COPY --chown=65533:65533 [".", "."]

FROM docker-registry.wikimedia.org/wikimedia-stretch:latest AS production
WORKDIR "/srv/service"
COPY --chown=65533:65533 [".", "."]
COPY --chown=65533:65533 --from=prep ["/go/src/gerrit.wikimedia.org/r/blubber/blubberoid", "./"]

In this case, I don't want the second to last line, the COPY . . but there's no way to omit it.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 10 2018, 8:03 PM

One refactoring option I can think of would be to make the config for copying of project/application files explicit and use a sane default.

e.g.

variants:
  build:
    copies: { hostfiles: true } # default value so not strictly needed here
  production:
    copies: { from: prep, hostfiles: false } # perhaps the default for `hostfiles:` could be based on `copies.from == nil` so it's not strictly necessary here either
  production2:
    copies: { hostfiles: false } # it would be necessary here since we're defining artifacts explicitly below
    artifacts:
    - from: prep
      source: /srv/app/binfile
      destination: ./

One refactoring option I can think of would be to make the config for copying of project/application files explicit and use a sane default.

e.g.

variants:
  build:
    copies: { hostfiles: true } # default value so not strictly needed here
  production:
    copies: { from: prep, hostfiles: false } # perhaps the default for `hostfiles:` could be based on `copies.from == nil` so it's not strictly necessary here either
  production2:
    copies: { hostfiles: false } # it would be necessary here since we're defining artifacts explicitly below
    artifacts:
    - from: prep
      source: /srv/app/binfile
      destination: ./

I like the idea of making copying of files explicit. In terms of syntax I think it makes sense to have a list of artifact structs that would now exist under copies. That is, I think the distinction between copies and artifacts is hard to grok. We could allow explicit src and dsts and when those are absent go with a sane default (i.e., for the localhost keyword COPY . .; for copying from variants copy lives.in -> lives.in + LocalLibPrefix -> LocalLibPrefix).

The "hostfiles" keyword is not obvious to me (I think of /etc/hosts for whatever reason). There isn't a great alternative. We could use . or localhost or hostfiles. Maybe hostfolder? I don't know.

I use localhost for the example below.

Example syntax:

build:
  builder: [do, a, thing, to, create, /bin/foo]
production:
  copies:
    - build:
       - src: /bin/foo
         dst: /bin/foo
---
build:
  builder: [do, a, thing, to, make, node_modules]
production:
  copies:
    - localhost
    - build
---
build:
  builder: [do, a, thing, to, make, node_modules]
  builder: [do, a, different, thing, to, create, /bin/foo]
production:
  copies:
    - localhost
    - build
    - build
      - src: /bin/foo
        dst: /bin/foo
---
build:
  copies:
    - localhost
  builder: [create, /bin/foo]
  builder: [populate, node_modules]
production:
  copies: [build]
dduvall added a comment.EditedDec 13 2018, 7:49 PM

@thcipriani +1 to the proposal in general. I think it adds clarity to artifact definition and to the purpose of copies. Just some notes on config structs and implementation of sane defaults.

Syntax

My initial reaction to allowing for both forms:

copies:
 - build

and:

copies:
 - build:
    - source: /bin/foo
    - destination: /bin/foo

is that it's nonobvious, and that reaction is probably from working with similar patterns in JJB. In JJB, I've never like the fact that it introduces essentially two more layers of nested structure (from list[string] to list[map[string: list[map[string: string]]]] to allow for the sake of a certain readable form when only one additional layer is needed to actually store the additional information (from list[string] to list[map[string: string]]).

Maybe that's a little pedantic, but I think there's a compromise that's still readable, which would be these two forms:

copies:
 - build

and:

copies:
 - from: build
   source: /bin/foo
   destination: /bin/foo

It still allows for a short form, which I think is important since it's close to what we currently support for copies (most compactly copies: [build]), and the long form is more efficient in its data representation and parsing.

Supporting either sets of short/long forms will require a little bit of extra logic in our parser, but I think we can and should do it for the sake of supporting the short form.

Defaults

For defaults, in order to be backwards compatible, we'd have to support the following interpretations/expansions. I'll use local in my examples to signify the host directory context, making it my official vote for a keyword. :)

Example

copies: [local]

should be expanded to

copies:
 - from: local
   source: .
   destination: .

Example

copies: [foo] # anything other than local

should be expanded to

copies:
 - from: foo
   source: /srv/app # assuming lives.in is left as the /srv/app default
   destination: /srv/app
 - from: foo
   source: /opt/lib
   destination: /opt/lib
thcipriani triaged this task as Normal priority.Dec 18 2018, 5:51 PM
thcipriani moved this task from Backlog to Doing on the Release Pipeline (Blubber) board.
dduvall renamed this task from Manually defining artifacts results in default copy of all project files to Unify configuration for local build-context copies and variant artifacts.Jan 18 2019, 10:46 PM
dduvall updated the task description. (Show Details)

Change 485340 had a related patch set uploaded (by Dduvall; owner: Dduvall):
[blubber@master] Unify copies and artifacts configuration

https://gerrit.wikimedia.org/r/485340

Change 485340 merged by jenkins-bot:
[blubber@master] Unify copies and artifacts configuration

https://gerrit.wikimedia.org/r/485340

thcipriani closed this task as Resolved.Feb 25 2019, 5:47 PM

I think @dduvall accomplished everything outlined on this task. Closing.