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

default enable watch bookmarks #226

Closed
clux opened this issue Apr 15, 2020 · 4 comments · Fixed by #445
Closed

default enable watch bookmarks #226

clux opened this issue Apr 15, 2020 · 4 comments · Fixed by #445
Labels
api Api abstraction related question Direction unclear; possibly a bug, possibly could be improved.

Comments

@clux
Copy link
Member

clux commented Apr 15, 2020

Currently, you have to select whether to pass on allowWatchBookmarks to the api server via ListParams, but you always have to have a match arm for it in when you get a WatchEvent from api.watch.

We could simplify this by having bookmarks be a compile time feature that makes the struct larger if enabled, as well as it automatically being enabled in ListParams::default.

Is this a too small use of a feature?

@clux clux added api Api abstraction related question Direction unclear; possibly a bug, possibly could be improved. labels Apr 15, 2020
@nightkr
Copy link
Member

nightkr commented Apr 15, 2020

I would flip this and ask: where would you ever want to disable bookmarks? If you care about bandwidth: you'll want to implement them. If you don't care about bandwidth, it's a trivial match arm to add, and to me it feels perfectly "Rusty" to have to add friction to slower options (just like Clone vs Copy). If you run <1.16, the apiserver will just ignore the flag anyway. If you uncover some pathological case for bookmarks.. you'll probably want to disable them in the control plane instead of hunting down and patching every last client.

@nightkr
Copy link
Member

nightkr commented Apr 15, 2020

Besides, there's the usual shpiel about cargo features causing breakage if they're not purely additive (since a sibling dependency might enable it for you, for example).

@clux
Copy link
Member Author

clux commented Apr 15, 2020

Yeah, convinced by that argument. Thank you.

So flipping this, perhaps we should just enable them in ListParams by default instead because we, as a client, implement support for them now, and you have to handle them anyway.

@clux clux added the wontfix This will not be worked on label Apr 15, 2020
@nightkr
Copy link
Member

nightkr commented Apr 15, 2020

Sounds good to me.

@clux clux changed the title bookmark feature gated? default enable watch bookmarks Feb 28, 2021
@clux clux removed the wontfix This will not be worked on label Feb 28, 2021
clux added a commit that referenced this issue Feb 28, 2021
Since this flag does nothing for older apiservers it's safe to default
enable it. This will also close #219 in the process.

Note that ListParams does not pick up on the flag for non-watch calls.
@clux clux linked a pull request Feb 28, 2021 that will close this issue
@clux clux closed this as completed in #445 Mar 1, 2021
clux added a commit that referenced this issue Mar 1, 2021
ListParams to default enable bookmarks - closes #226
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Api abstraction related question Direction unclear; possibly a bug, possibly could be improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants