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

Refine ko builder behavior for main packages #6437

Merged
merged 2 commits into from
Aug 16, 2021

Conversation

halvards
Copy link
Collaborator

Description
Decide on how ko builder users should specify the location of package main.

Previous discussion: #6054 (comment)

Tracking: #6041
Related: #6054, #6286

Decide on how ko builder users should specify the location of
`package main` for their images.

Previous discussion: GoogleContainerTools#6054 (comment)

Tracking: GoogleContainerTools#6041
Related: GoogleContainerTools#6054, GoogleContainerTools#6286
@halvards halvards requested a review from a team as a code owner August 16, 2021 05:10
@google-cla google-cla bot added the cla: yes label Aug 16, 2021
@codecov
Copy link

codecov bot commented Aug 16, 2021

Codecov Report

Merging #6437 (7fde494) into main (9535940) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 7fde494 differs from pull request most recent head edc54d1. Consider uploading reports for the commit edc54d1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6437      +/-   ##
==========================================
+ Coverage   70.40%   70.41%   +0.01%     
==========================================
  Files         505      505              
  Lines       22840    22840              
==========================================
+ Hits        16080    16083       +3     
+ Misses       5708     5705       -3     
  Partials     1052     1052              
Impacted Files Coverage Δ
pkg/skaffold/docker/parse.go 87.39% <0.00%> (-0.85%) ⬇️
pkg/skaffold/util/tar.go 65.51% <0.00%> (+2.29%) ⬆️
pkg/skaffold/log/stream/stream.go 85.71% <0.00%> (+14.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9535940...edc54d1. Read the comment docs.

@@ -263,6 +291,13 @@ Adding the ko builder requires making config changes to the Skaffold schema.
// You can override this value by setting the `SOURCE_DATE_EPOCH`
// environment variable.
SourceDateEpoch uint64 `yaml:"sourceDateEpoch,omitempty"`

// Target is the location of the main package.
// If target is specified as a relative path, it is relative to the `context` directory.
Copy link
Member

Choose a reason for hiding this comment

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

Wld ko error out if two main are found in the context dir?

Copy link
Member

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

LGTM except for nit.

Clarify how a blank value works, and also how users can use a pattern
with wildcards.
@halvards
Copy link
Collaborator Author

Thanks @tejal29! I pushed a commit with an improved explanation.

@tejal29 tejal29 merged commit f88c88e into GoogleContainerTools:main Aug 16, 2021
@halvards halvards deleted the ko-import-path-design branch August 16, 2021 23:50
@halvards
Copy link
Collaborator Author

Thanks for the quick turn-around!

halvards added a commit to halvards/skaffold that referenced this pull request Aug 17, 2021
This implements support for the Target config field for the ko builder.
The Target field allows users to specify a target for `go build`. This
is necessary where the main package is not in the context directory.

Tracking: GoogleContainerTools#6041
Related: GoogleContainerTools#6054, GoogleContainerTools#6437
tejal29 pushed a commit that referenced this pull request Aug 20, 2021
This implements support for the Target config field for the ko builder.
The Target field allows users to specify a target for `go build`. This
is necessary where the main package is not in the context directory.

Tracking: #6041
Related: #6054, #6437
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.

2 participants