Add file sync to Event and State APIs

[Multiply]

The problem we're facing
We're running a local builder, and we sync the built files to our container, to then serve them to the browser.
As soon as the local builder is done, it refreshes the browser, expecting it to be ready to be served, but in most instances (read; almost all instances), Skaffold hasn't started, or is not done with the file sync.

Our current solution
Add a timeout in our local builder, to reload the browser after a second. This slows down development.

Our proposed solution with the help of this PR
Listen for new Skaffold events over gRPC or plain HTTP, and reload the browser as soon as we see FileEventSync with a Status of Complete.

The problem we have at the moment, is the browser reloads as soon as the local build tool is done, but before the files have reached the pod, so we see no changes in the browser.
With this, we can follow skaffold events in our build-chain when in watch-mode, and reload the browser as soon as Skaffold informs us that it's ready.
Without this, we have to resort to odd timeouts before doing a reload, which results in poor developer experience.

This spawned from a short chat on Slack, where we needed to reload our browsers as soon as files built on the host machine has landed inside the pods, and is ready to be served.
I forgot to create an issue after we had the initial talk, so please let me know if I need to still do that.

---

Relates to Event and State APIs

Should merge before : #3009

Description
Add FileSyncEvent that notifies about the status, file count, and image for a given file sync.
Add FileSyncState that keeps state of current file sync.

User facing changes
Before
No events are fired when file sync is triggered.
No state available for current file sync.

After
FileSyncEvent with status InProgress is fired, when a file sync is triggered.
FileSyncEvent with status Failed is fired, if a file sync fails.
FileSyncEvent with status Complete is fired, when a file sync is complete without errors.
FileSyncState is updated when the events above are fired.

Next PRs.
#3009

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

New Features
- Add file sync to Event and State APIs

[codecov]

Codecov Report

Merging #2978 into master will increase coverage by 0.07%.
The diff coverage is 96.66%.

Impacted Files	Coverage Δ
pkg/skaffold/event/event.go	92.7% <96.66%> (+0.48%)	⬆️

[tejal29]

Thanks @Multiply for your PR.

This looks great! Can we break this in 2 PRs. Please see our in progress small PR guidelines
#2977

1. With the new proto definition and event Methods.
2. Integrate it with the dev flow.

That would help us concentrate on smaller changes.

Comments for the proto:

1. Do you plan to send event per file synced in a container?
   • in that case should we have the filename instead of file count
   • or the copies are too fast and it should not matter?
2. Having filename in the event will help users identify which file synced successfully and which did not so they do understand why it failed. Based on that information, they could change their sync definition if its wrong or copy the file manually. WDYT?
   For the skaffold/event/event.go, Can you add methods
   i. InProgress
   ii. Succeeded
   iii. Failed.
   See Use enum instead of string for "InProgress" , "Succeeded", "Started", "Failed" #2967

[Multiply]

Can we break this in 2 PRs?

Sure. I'll get on that, when we have an alignment on the proto definition (with regards to files etc.)

1. Do you plan to send event per file synced in a container?

For our specific use case, we don't need the actual files, that was changed, but merely that a sync happened. (like the one you see in the Skaffold output when it's watching for changes)
There's legitimate use cases for knowing what changed, but in that case, I think it should be one event per "file sync", ie. multiple files batched into one request, and not one event per change. (there'd be too much noice, not batching them.

2. Having filename in the event will help users identify which file synced successfully and which did not so they do understand why it failed. Based on that information, they could change their sync definition if its wrong or copy the file manually. WDYT?

Based on my answer above, which format do you propose? One event, with a Changes, and deletes?
I can definitely add that behaviour, but it will increase the scope of the feature.

For the skaffold/event/event.go, Can you add methods
i. InProgress
ii. Succeeded
iii. Failed.
See #2967

Do you just want me to rename my FileEventComplete to FileEventSucceeded?

[tejal29]

Can we break this in 2 PRs?

Sure. I'll get on that, when we have an alignment on the proto definition (with regards to files etc.)

1. Do you plan to send event per file synced in a container?

For our specific use case, we don't need the actual files, that was changed, but merely that a sync happened. (like the one you see in the Skaffold output when it's watching for changes)
There's legitimate use cases for knowing what changed, but in that case, I think it should be one event per "file sync", ie. multiple files batched into one request, and not one event per change. (there'd be too much noice, not batching them.

so, i re-read the sync code and looks like we create a tar of all files changes in an image and only execute 1 sync. So, we can't have one sync event per file, so we can only do one sync event per image and thats what your proposed implementation does. Lets not change it.

1. Having filename in the event will help users identify which file synced successfully and which did not so they do understand why it failed. Based on that information, they could change their sync definition if its wrong or copy the file manually. WDYT?

Based on my answer above, which format do you propose? One event, with a Changes, and deletes?
I can definitely add that behaviour, but it will increase the scope of the feature.

I think for now let's not include the filename. We could support this in future, especially if tools like Cloud Code (they consume events) need information on which files were part of the failed sync and which container so they can potentially suggest a fixable action.

Do you just want me to rename my FileEventComplete to FileEventSucceeded?

yes

[Multiply]

@tejal29 I've changed Complete to Succeeded, and added checks for In Progress and Succeeded events in the integration tests.

Following the discussion in #2977, do you still want me to split it up into two separate PRs?

[tejal29]

@tejal29 I've changed Complete to Succeeded, and added checks for In Progress and Succeeded events in the integration tests.

Following the discussion in #2977, do you still want me to split it up into two separate PRs?

yes please. Thank you!

[Multiply]

@tejal29 I've rebased this PR to only include the proto changes and event methods.
I also opened #3009 with the dev flow integration.