Update skaffold init --artifact to use JSON structs instead of paths

[TadCordle]

Follow up to #2276 (last one probably).

This PR makes changes to skaffold init --artifact to allow passing in non-Docker builders via CLI. Rather than passing a path/image pair, which doesn't provide enough information for some builders, --artifact now takes a JSON struct/image pair. Example below:

skaffold init \
    -a='{"name":"Docker","payload":{"path":"notification-service/Dockerfile"},"image":"gcr.io/docker/image"}' \
    -a='{"name":"Jib Gradle Plugin","payload":{"path":"vote-service/build.gradle"},"image":"gcr.io/gradle/image"}'

This new format isn't very user friendly due to the verbosity of the JSON (although the old method of path=image is still available), but it should make passing non-docker builder information from IDEs using a skaffold init --analyze --> skaffold init --artifact=... workflow fairly straightforward.

[chanseokoh]

Just asking... won't /web/Dockerfile.web read more natural without the leading /? It may usually be a relative path? I can't remember exactly, but can this handle absolute path properly?

[chanseokoh]

Looks good to me.

Comments addressed

[nkubala]

in principle I think I'm fine with this approach. the JSON fields feel a little weird to me though.

--artifact '{"name": "Docker", "payload": {"path": "/path"}}=gcr.io/repo/image'

this reads to me like "the name of this artifact is Docker" rather than "the name of the builder this artifact is meant for is Docker", and it's confusing what the "payload" is. also, we have this entire JSON struct "equal to" the image name, so it's just kind of hard to parse out the semantics of what is actually being provided to skaffold.

WDYT about moving the image name into the JSON payload itself? then we can change the name field to builder, get rid of the payload field and just have the path and the image inside the JSON.

--artifact '{"builder": "Docker", "path": "/path", "image": "gcr.io/repo/image"}'

this feels a little more clear to me, and should still work with the backwards compatibility logic.

[TadCordle]

@nkubala The reason there's a separate "payload" field rather than just putting the rest of the fields in one flat json struct is because different builders have different builder-specific configuration values. Things like jib's module name, or build system (maven/gradle), or jib's to.image configuration don't apply if you're using a builder like Docker, which only uses a path to a dockerfile. This is why we went with the design of determining the builder type using the "name" field, then correctly parsing "payload" based on the type, rather than just having every configuration value for every builder mixed together in the same struct (which I think would become really messy, especially if more builders are added to skaffold init).

I do agree about changing "name" to "builder", and moving the image after the '=' into the json as well.

[nkubala]

LGTM when CI is green

[TadCordle]

RUN hack/check-docs.sh
There are        1 changes in docs, testing site generation...
./deploy/docs/local-preview.sh hugo --baseURL=https://skaffold.dev
./deploy/docs/local-preview.sh: line 29: docker: command not found
make[1]: *** [build-docs-preview] Error 127
FAILED hack/check-docs.sh

Is there some command I can run to fix this? This doesn't happen locally.

[nkubala]

yeah i'm not really sure why that's happening....i've never seen this happen on any other PRs, but it seems like a travis issue. let's try rerunning and see if it fixes itself

[TadCordle]

No luck. Looks like it's just mac too.