-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Ko builder: Implement file watch dependencies #6617
Ko builder: Implement file watch dependencies #6617
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6617 +/- ##
==========================================
- Coverage 70.48% 70.02% -0.46%
==========================================
Files 515 523 +8
Lines 23150 23747 +597
==========================================
+ Hits 16317 16629 +312
- Misses 5776 6035 +259
- Partials 1057 1083 +26
Continue to review full report at Codecov.
|
60c7850
to
f2e243f
Compare
f2e243f
to
99c06f6
Compare
// defaultKoDependencies behavior is to watch all files in the context directory | ||
func defaultKoDependencies() *latestV1.KoDependencies { | ||
return &latestV1.KoDependencies{ | ||
Paths: []string{"."}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shd stay away from having .
as default dependency for following reasons
- A change in
.git/*.lock
file will trigger a dev loop - A change in any hidden files (like .swap files or .ides) will trigger dev loop.
Can we instead add **/*.go
as default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tejal29, good point! I've pushed a new commit that uses **/*.go
as the default. We'll need the globstar support in #6605 to make this pattern work well.
As a side note, the buildpacks and custom builders both use .
as the default:
Paths: []string{"."}, |
For those builders we can't really list patterns, since it could be any number of languages. Should we consider a list of default excludes for those builders?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except default dependency
Add support for the `dependencies` section in the ko builder config. Use `**/*.go` instead of `.` as the default file watch pattern for the ko builder. As @tejal29 [points out](GoogleContainerTools#6617 (comment)), this prevents unintended image rebuilds based on changes to files in `.git/`, `.idea/`, and `*.swap` files and similar. For this pattern to be effective, the changes in GoogleContainerTools#6605 are required. **Tracking:** GoogleContainerTools#6041 **Related:** GoogleContainerTools#6605
99c06f6
to
318f83e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Description
Add support for the
dependencies
section in the ko builder config.Use
**/*.go
as the default file watch pattern, this prevents unintended image rebuilds based on changes to files in.git/
,.idea/
, and*.swap
files and similar.For this pattern to be effective, the changes in #6605 are required.
Tracking: #6041
Related: #6605