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

Improve skaffold init file traversal #3062

Merged
merged 9 commits into from
Oct 21, 2019
Merged

Improve skaffold init file traversal #3062

merged 9 commits into from
Oct 21, 2019

Conversation

TadCordle
Copy link
Contributor

@TadCordle TadCordle commented Oct 15, 2019

Fixes #3060.

Description

Currently, skaffold init ends directory traversal early when a Jib config is detected, so as to not accidentally traverse sub-modules, detect more Jib configurations, and end up with a list of redundant builders. However, the directory traversal had issues that could cause other important files to be missed, such as k8s configs, existing skaffold.yamls, or other build files in the Jib config directory (see before/after descriptions below). This PR changes skaffold init to fully walk the project directory, using a flag to ignore builders in nested directories instead of ignoring the directories altogether. This lets us find k8s manifests/skaffold.yamls regardless of where they are relative to Jib build files, while letting us ignore redundant builder configs.

User facing changes

skaffold init will now detect important files more consistently.

Before

Given the example project directory below:

Dockerfile
build.gradle  <-- Jib configured here
kubernetes-manifests/
pom.xml
skaffold.yaml
src/
target/

skaffold init would traverse the directory in alphabetical order, finding the dockerfile first, then finding Jib configured in the build.gradle first, causing it to stop and return to the parent directory. This causes it to miss another potential Jib config in the same directory (pom.xml), the kubernetes manifests, and the existing skaffold.yaml.

After

Now, skaffold init traverses all the files in a directory first before traversing sub-directories (solving the problem of missed builders/skaffold.yaml). If a Jib config is found, then it will not continue to search for builders in the sub-directories; however it will continue to traverse deeper to find k8s manifests (e.g. in kubernetes-manifests/).

Submitter Checklist

  • Includes unit tests
  • Mentions any output changes.
  • Adds documentation as needed: user docs, YAML reference, CLI reference.
  • Adds integration tests if needed.

Reviewer Notes

  • The code flow looks good.
  • Unit test added.
  • User facing changes look good.

Release Notes

Fix skaffold init ending file traversal early when Jib config is detected.

@codecov
Copy link

codecov bot commented Oct 15, 2019

Codecov Report

Merging #3062 into master will increase coverage by 0.2%.
The diff coverage is 85.36%.

Impacted Files Coverage Δ
pkg/skaffold/initializer/init.go 57.19% <85.36%> (-0.27%) ⬇️
pkg/skaffold/build/cluster/logs.go 16.66% <0%> (-3.34%) ⬇️
pkg/skaffold/build/bazel/dependencies.go 78.43% <0%> (-2.34%) ⬇️
cmd/skaffold/app/cmd/config.go 100% <0%> (ø) ⬆️
pkg/skaffold/build/parallel.go 94.36% <0%> (ø) ⬆️
pkg/skaffold/build/local/local.go 33.33% <0%> (ø) ⬆️
pkg/skaffold/jib/jib_gradle.go 100% <0%> (ø) ⬆️
pkg/skaffold/build/custom/dependencies.go 83.33% <0%> (ø) ⬆️
pkg/skaffold/build/cluster/secret.go 36.2% <0%> (ø) ⬆️
... and 80 more

@TadCordle
Copy link
Contributor Author

@tejal29 I broke the long function down, LMK if it's an improvement.

Copy link
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

The overall logic part looks sound to me. Some minor naming suggestions.

@TadCordle
Copy link
Contributor Author

@tejal29 @nkubala Would you mind giving another review? @chanseokoh's approval isn't enough since this is outside of Jib files.

@nkubala nkubala merged commit a287c25 into GoogleContainerTools:master Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skaffold init with Jib init behaves differently in analyze mode
5 participants