fix: .gitignore .func #1728

lkingland · 2023-05-09T06:05:32Z

🐛 .gitignore file path incorrect when creating in a different path that CWD
🐛 .gitignore /.func was causing the build stamp to update
🎁 can now force inclusion of .func in source control by commenting the line
🎁 adds built log journaling via a WithStampJournal option

/kind bug
/kind enhancement

Fixes an issue where .gitignore was not correctly updated when running func with a custom --path
Improves build caching

codecov · 2023-05-09T06:13:44Z

Codecov Report

Patch coverage: 66.42% and project coverage change: +10.10 🎉

Comparison is base (e9385f3) 49.66% compared to head (128fc0e) 59.76%.

❗ Current head 128fc0e differs from pull request most recent head 014ef00. Consider uploading reports for the commit 014ef00 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1728       +/-   ##
===========================================
+ Coverage   49.66%   59.76%   +10.10%     
===========================================
  Files          82       94       +12     
  Lines       11474    12343      +869     
===========================================
+ Hits         5698     7377     +1679     
+ Misses       5281     4285      -996     
- Partials      495      681      +186

Flag	Coverage Δ
e2e-test-oncluster-runtime	`29.34% <44.28%> (?)`
e2e-test-runtime-go	`27.90% <44.28%> (?)`
e2e-test-runtime-quarkus	`29.17% <44.28%> (?)`
e2e-test-runtime-springboot	`27.99% <44.28%> (?)`
e2e-test-runtime-typescript	`29.17% <44.28%> (?)`
integration-tests	`50.21% <44.28%> (?)`
unit-tests-ubuntu-latest	`49.75% <66.42%> (+0.09%)`	⬆️
unit-tests-windows-latest	`48.91% <66.42%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/functions/client.go	`63.57% <62.93%> (+7.04%)`	⬆️
pkg/functions/function.go	`84.56% <83.33%> (+5.21%)`	⬆️

... and 48 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

pkg/functions/function_test.go

lance · 2023-05-09T17:00:44Z

@lkingland the linter fails with

 Error: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred in new code. See the specific function documentation for details.  (staticcheck)

lkingland · 2023-05-10T02:18:54Z

pkg/functions/client_test.go

-// TestClient_BuiltDetects ensures that the client's Built command detects
-// filesystem changes as indicating the function is no longer Built (aka stale)
-// This includes modifying timestamps, removing or adding files.
-func TestClient_BuiltDetects(t *testing.T) {


This test was just moved in its entirety to be with the other functions tests, since the Built logic is now a method on Function rather than a package static function.

lkingland · 2023-05-10T02:24:22Z

pkg/functions/function.go

@@ -394,19 +408,46 @@ func (f Function) Write() (err error) {
 //
 // The runtime data directory .func is created in the function root if
 // necessary.
-func (f Function) Stamp() (err error) {
+func (f Function) Stamp(oo ...stampOption) (err error) {


Journaling is very much an optional, debugging-focused feature, so I used the option pattern here to retain the default API as the happy-path

lkingland · 2023-05-11T16:56:49Z

pkg/functions/client.go

@@ -960,6 +966,178 @@ func (c *Client) Push(ctx context.Context, f Function) (Function, error) {
 	return f, nil
 }

+// ensureRunDataDir creates a .func directory at the given path, and
+// registers it as ignored in a .gitignore file.
+func ensureRunDataDir(root string) error {


These functions (ensureRunDataDir, fingerprint, ...) support the functionality of the Client methods, so they were moved from function.go. Keeping Function as close to a data object as possible, with Client being the actor. The only one appreciably modified was this one, ensureRunDataDir.

lkingland · 2023-05-11T16:58:43Z

pkg/functions/client_test.go

@@ -51,6 +52,138 @@ func TestClient_New(t *testing.T) {
 	}
 }

+// TestClient_New_RunData ensures that the .func runtime directory is
+// correctly created.
+func TestClient_New_RunDataDir(t *testing.T) {


This is the new test

- The .gitignore file was always relative to process' current working directory. Now correctly uses the function root - The .gitignore file was always being updated, causing its modification timestamp to be updated multiple times throughout the executaion of any client commands. - Adds the ability to override this behavior by commenting out the line in the .gitignore. - Adds the ability to request that stamping create an ongoing journal via a build log file with timestamp prefix (for debugging)

matejvasek · 2023-05-16T17:23:24Z

Running func run twice in a row do build also on the second run, I think that should not be happening.

matejvasek · 2023-05-16T17:33:28Z

Running func run twice in a row do build also on the second run, I think that should not be happening.

Although this might not be caused by this PR.
EDIT: it is not caused by this PR, it was probably broken some time ago.
Deploy does the same unnecessary rebuild, I would swear this worked at some point.

lkingland · 2023-05-16T17:50:01Z

Running func run twice in a row do build also on the second run, I think that should not be happening.

Although this might not be caused by this PR. EDIT: it is not caused by this PR, it was probably broken some time ago. Deploy does the same unnecessary rebuild, I would swear this worked at some point.

Thanks for taking a look. Yes caching is something that is still not happening with complete reliability. This PR gets it working in a few more scenarios and makes it more auditable so we can debug the others more easily. In particular it helps enable the changes to run in #1733. I would expect caching to be working as part of that PR

matejvasek · 2023-05-16T19:40:39Z

/approve
/lgtm

knative-prow · 2023-05-16T19:40:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, matejvasek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [lkingland,matejvasek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow bot requested review from jrangelramos and nainaz May 9, 2023 06:05

knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 9, 2023

lkingland marked this pull request as ready for review May 9, 2023 09:31

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 9, 2023

knative-prow bot requested a review from rhuss May 9, 2023 09:31

lkingland removed request for rhuss, jrangelramos and nainaz May 9, 2023 09:31

lkingland force-pushed the fix-gitignore-func branch from 7b0e672 to 27d36f3 Compare May 9, 2023 11:22

lkingland mentioned this pull request May 9, 2023

feat: oci directory pusher #1702

Merged

lance reviewed May 9, 2023

View reviewed changes

pkg/functions/function_test.go Outdated Show resolved Hide resolved

lkingland commented May 10, 2023

View reviewed changes

lkingland force-pushed the fix-gitignore-func branch 3 times, most recently from 8bda323 to ad45e9c Compare May 11, 2023 05:54

lkingland requested a review from lance May 11, 2023 12:58

knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 11, 2023

lkingland force-pushed the fix-gitignore-func branch from 08865da to 9e1f52d Compare May 11, 2023 14:53

lkingland commented May 11, 2023

View reviewed changes

lkingland requested a review from zroubalik May 13, 2023 05:10

lkingland requested review from salaboy, matejvasek and aslom May 13, 2023 05:14

lkingland force-pushed the fix-gitignore-func branch from 128fc0e to 014ef00 Compare May 13, 2023 06:29

knative-prow bot assigned matejvasek May 16, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 16, 2023

knative-prow bot merged commit 94c81d4 into knative:main May 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: .gitignore .func #1728

fix: .gitignore .func #1728

lkingland commented May 9, 2023

codecov bot commented May 9, 2023 •

edited

Loading

lance commented May 9, 2023

lkingland May 10, 2023 •

edited

Loading

lkingland May 10, 2023

lkingland May 11, 2023 •

edited

Loading

lkingland May 11, 2023

matejvasek commented May 16, 2023 •

edited

Loading

matejvasek commented May 16, 2023 •

edited

Loading

lkingland commented May 16, 2023 •

edited

Loading

matejvasek commented May 16, 2023

knative-prow bot commented May 16, 2023

fix: .gitignore .func #1728

fix: .gitignore .func #1728

Conversation

lkingland commented May 9, 2023

codecov bot commented May 9, 2023 • edited Loading

Codecov Report

lance commented May 9, 2023

lkingland May 10, 2023 • edited Loading

Choose a reason for hiding this comment

lkingland May 10, 2023

Choose a reason for hiding this comment

lkingland May 11, 2023 • edited Loading

Choose a reason for hiding this comment

lkingland May 11, 2023

Choose a reason for hiding this comment

matejvasek commented May 16, 2023 • edited Loading

matejvasek commented May 16, 2023 • edited Loading

lkingland commented May 16, 2023 • edited Loading

matejvasek commented May 16, 2023

knative-prow bot commented May 16, 2023

codecov bot commented May 9, 2023 •

edited

Loading

lkingland May 10, 2023 •

edited

Loading

lkingland May 11, 2023 •

edited

Loading

matejvasek commented May 16, 2023 •

edited

Loading

matejvasek commented May 16, 2023 •

edited

Loading

lkingland commented May 16, 2023 •

edited

Loading