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

RFC: deprecate and remove Watch Mode #412

Closed
imjasonh opened this issue Aug 9, 2021 · 8 comments
Closed

RFC: deprecate and remove Watch Mode #412

imjasonh opened this issue Aug 9, 2021 · 8 comments

Comments

@imjasonh
Copy link
Member

imjasonh commented Aug 9, 2021

Watch Mode (i.e, ko apply --watch) was added about 2 1/2 years ago, in the old ggcr repo, in google/go-containerregistry#336.

We don't collect usage metrics, but based on recommended/documented usage in projects that I know of that use ko, none use watch mode today. It was marked as EXPERIMENTAL from the beginning, and documentation for it was apparently accidentally removed from the README six months ago in #318 😞.

Watch Mode adds quite a bit of complexity to what is otherwise a relatively straightforward transformation on inputs. It involves implementing non-standard concurrency primitives like Futures. It's not well tested, and not currently easily test-able. It might make future improvements to outputs like #406 and ko extract more difficult, or simply incompatible with watch mode.

With support for watching and re-building images with ko available through Skaffold (today unofficially; in the future, officially), and upcoming improvements to speed up no-op builds (#267 #269) I think we should consider deprecating and eventually removing watch mode from ko, simplifying the codebase and focusing efforts on better release management integration and security best practices.

If we decide to proceed, we can start by adding a warning linking this issue when using --watch, to collect any feedback that might encourage us to reconsider deprecating. If we do decide to keep it, we should invest more in documentation and testing, and promote it out of an undocumented experiment.

wdyt @mattmoor @jonjohnsonjr

@jonjohnsonjr
Copy link
Collaborator

If we decide to proceed, we can start by adding a warning linking this issue when using --watch, to collect any feedback that might encourage us to reconsider deprecating.

This seems funny to me. If --watch is set, print something like Come argue in issue 412 if you use this.

On the other hand, I think I like the future stuff. It is pretty nice for streaming support (ko resolve -f -).

@imjasonh
Copy link
Member Author

imjasonh commented Aug 9, 2021

Honestly I'm fine with just removing it, I just want to give people an opportunity to tell us why they think it's load-bearing for them, if it is.

I'm not sure I can think of a case where we wouldn't be able to just buffer all of stdin then process it all non-streamily -- it's one YAML Michael, how much of it could there be, 10 MB? But you're right, it's easy enough to support today so why not.

@jonjohnsonjr
Copy link
Collaborator

I'm not sure I can think of a case where we wouldn't be able to just buffer all of stdin

curl my-cluster.example.com/ko/out-of-date-image-stream | ko resolve -f -

This is obviously contrived, but imagine you have a service on your cluster that streams any yaml documents containing out of date images on that cluster and doesn't close the request until the images have been updated. ko apply -f - would update those deployments, which would cause the yaml stream to finally close.

😄

@imjasonh
Copy link
Member Author

imjasonh commented Aug 9, 2021

This is obviously contrived.

🙃

@mattmoor
Copy link
Collaborator

Do it, it doesn't work well enough on macOS, so I haven't used it since I left Google, and I'm not sure I ever tested it after some of Jon's go module changes.

@github-actions
Copy link

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

@imjasonh
Copy link
Member Author

We still haven't removed Watch Mode.

fsnotify, which we depend on for watch mode, is in need of maintainers, and if some aren't identified soon the repo will be archived: fsnotify/fsnotify#413

@imjasonh
Copy link
Member Author

imjasonh commented Apr 4, 2023

#585

@imjasonh imjasonh closed this as completed Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants