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

filewatch: perform double writes to store + apiserver #4307

Merged
merged 2 commits into from
Mar 17, 2021

Conversation

milas
Copy link
Contributor

@milas milas commented Mar 12, 2021

There's a variety of changes here to actually ensure that the
created FileWatch objects are actually acceptable by apiserver
(e.g. : isn't a valid character in labels, so I split it into
target type + target name labels instead of playing string split
games).

With these changes, the file watches are now visible in the API
server.

Need to merge + bump dep for tilt-dev/tilt-apiserver#26 before merge

@milas milas requested review from nicks and vkorbes March 12, 2021 16:28
@milas milas marked this pull request as ready for review March 12, 2021 16:48
@milas milas marked this pull request as draft March 12, 2021 16:55
pkg/apis/core/v1alpha1/register.go Outdated Show resolved Hide resolved
@milas milas force-pushed the milas/filewatch-double-write branch 2 times, most recently from 9ee7350 to e020df5 Compare March 15, 2021 18:19
There's a variety of changes here to actually ensure that the
created FileWatch objects are actually acceptable by apiserver.

With these changes, the file watches are now visible in the API
server!
@milas milas force-pushed the milas/filewatch-double-write branch from e020df5 to eb2f64e Compare March 16, 2021 18:07
@milas milas marked this pull request as ready for review March 16, 2021 19:13
For now, simply hold onto a reference to the object to update
rather than fetching from apiserver. This hopefully prevents
race issues until everything is fully in apiserver.
@milas milas requested a review from nicks March 16, 2021 19:22
@milas
Copy link
Contributor Author

milas commented Mar 16, 2021

@nicks Mind taking a quick pass at this again? It should be mostly the same with one exception other than bringing up-to-date with master (sorry, I did some rebasing so no useful history).

The exception is ec5ff3e - I changed to hold onto a reference of the object to use for updates vs fetching from apiserver; I've been seeing suspicious elevated rates of integration tests failures that were making me nervous, so this seemed safer until it's fully in apiserver.

Copy link
Member

@nicks nicks left a comment

Choose a reason for hiding this comment

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

lgtm!

@milas milas merged commit d454919 into master Mar 17, 2021
@milas milas deleted the milas/filewatch-double-write branch March 17, 2021 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants