Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update skaffold init --analyze to handle more builder types #2327

Merged
merged 28 commits into from
Jul 11, 2019
Merged

Update skaffold init --analyze to handle more builder types #2327

merged 28 commits into from
Jul 11, 2019

Conversation

TadCordle
Copy link
Contributor

@TadCordle TadCordle commented Jun 24, 2019

This is a follow-up to #2274.

Currently, skaffold init --analyze only shows dockerfiles and k8s images found, which isn't useful for non-dockerfile-based builder detection. This PR updates the schema to provide more useful information for IDEs to consume.

This PR:

  • Renames the "dockerfiles" field to more general "builders" and changes it to be a list of structs containing a "name" and "payload" field rather than just a list of paths
    • "name" is the name of the builder that was detected, e.g. "Docker", "Jib", etc.
    • "payload" is the information parsed from the build configuration, which will vary depending on which builder was detected
  • Changes "images" from a list of strings to a list of structs with a "name" and "promptForArtifactSource" field
    • "name" is the name of the image
    • "promptForArtifactSource" was added to allow IDEs to easily identify which builder/image pairs were automatically resolved (false) and which pairs will require prompting from the user (true) (with the addition of the Jib builder, some builder/image pairs will be automatically resolved).

Previous output:

{
    “dockerfiles”:[”/path/to/Dockerfile”, “another/Dockerfile”, ...],
    ”images”:[“gcr.io/project/image”, “another/image”, …],
}

Example new output:

{
    “builders”:[
        {
            “name”:“Docker”,
            “payload”:{“path”: “path/to/Dockerfile”},
        },
        {
            “name”:“Jib Gradle Plugin”,
            “payload”:{
                “path”:“path/to/build.gradle”,
                “configuredImage”:“gcr.io/project/image”,
                “project”:“child-project-name”
            },
        },
        ...
    ],
    “images”:[
        {“name”:”gcr.io/project/image”, “promptForArtifactSource”:”false”},
        {“name”:”another/image”, “promptForArtifactSource”:”true”},
    ]
}

@codecov
Copy link

codecov bot commented Jun 24, 2019

Codecov Report

Merging #2327 into master will increase coverage by 4.36%.
The diff coverage is 66.66%.

Impacted Files Coverage Δ
pkg/skaffold/docker/docker_init.go 93.33% <100%> (-0.22%) ⬇️
cmd/skaffold/app/cmd/init.go 56.52% <22.22%> (+1.52%) ⬆️
pkg/skaffold/initializer/init.go 43.98% <72.09%> (+5.15%) ⬆️
pkg/skaffold/docker/remote.go 35.41% <0%> (-13.16%) ⬇️
pkg/skaffold/docker/image.go 70.58% <0%> (-5.21%) ⬇️
pkg/skaffold/schema/v1beta9/upgrade.go 85.71% <0%> (-3.33%) ⬇️
cmd/skaffold/app/cmd/config.go 100% <0%> (ø) ⬆️
cmd/skaffold/app/cmd/commands.go 100% <0%> (ø) ⬆️
pkg/skaffold/kubernetes/context/context.go 100% <0%> (ø) ⬆️
pkg/skaffold/docker/image_util.go 0% <0%> (ø) ⬆️
... and 30 more

@TadCordle TadCordle requested a review from a team June 25, 2019 18:40
@loosebazooka
Copy link
Member

@nkubala any chance you could look at this, given you familiarity with skaffold init?

@chanseokoh
Copy link
Member

Looks good. I believe the VS code team is in sync with the schema change?

@TadCordle
Copy link
Contributor Author

@chanseokoh Yeah, although there's one request to change Docker's payload to be in the form of {"path": "path/to/Dockerfile"} rather than just "path/to/Dockerfile", so I think I'll change that real quick.

@TadCordle
Copy link
Contributor Author

@sujit-kamireddy Feel free to take a look and suggest changes as well.

@sujit-kamireddy
Copy link

Merging this will break the IDEs due to the new output format. Thanks @balopat for bringing this up. Please hold on merging this till we lock on the plan.

Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few nits

pkg/skaffold/initializer/init_test.go Outdated Show resolved Hide resolved
pkg/skaffold/initializer/init_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We agreed on to put this behind a hidden feature flag to keep backward compatibility for now with the Cloud Code extensions.

Copy link
Contributor Author

@TadCordle TadCordle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@balopat done

docs/content/en/docs/references/cli/_index.md Outdated Show resolved Hide resolved
@TadCordle TadCordle requested a review from balopat July 11, 2019 16:50
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit on the nit fixes then it's LGTM!

pkg/skaffold/initializer/init.go Show resolved Hide resolved
@TadCordle TadCordle merged commit d8bf283 into GoogleContainerTools:master Jul 11, 2019
@TadCordle TadCordle deleted the update-skaffold-init-analyze branch July 11, 2019 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants