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

auto-discovers should always add new bindings #1674

Open
psFried opened this issue Oct 3, 2024 · 7 comments
Open

auto-discovers should always add new bindings #1674

psFried opened this issue Oct 3, 2024 · 7 comments

Comments

@psFried
Copy link
Member

psFried commented Oct 3, 2024

Currently auto-discovers will only add bindings if addNewBindings is enabled. We'd like to instead have them always add the new bindings, and to have them be disabled if addNewBindings is false. This allows users to selectively enable bindings later.

@dyaffe
Copy link
Member

dyaffe commented Oct 3, 2024

Something even better here could be having a second discovery mode which doesn't add new bindings by default. The UI could then use that when addNewBindings is set to false

@psFried
Copy link
Member Author

psFried commented Oct 3, 2024

Something even better here could be having a second discovery mode which doesn't add new bindings by default. The UI could then use that when addNewBindings is set to false

We literally already have this. It's the discovers.update_only column.

@dyaffe
Copy link
Member

dyaffe commented Oct 3, 2024

Amazing. So could a simple solution just be a UI change to call that when someone has the addNewBindings set to false? That would accomplish the goal.

If so, I would suggest we update this to a high priority frontend issue.

@psFried
Copy link
Member Author

psFried commented Oct 3, 2024

@dyaffe That's what I'd been thinking, too. But @jonwihl made what I think is a pretty good point. Say a user is capturing from a database with a ton of tables, and they're only capturing a couple of them. Now they want to add a table. If our UI just presents two different options for how to refresh bindings, then neither would provide an easy solution for just adding a small number of possible tables.

But we could serve that use case well if we instead change discovers to always generate models that represent all bindings, and just keep them disabled if addNewBindings is false. And we already have enable|disable-all buttons in case the user does actually want to enable all the new bindings.

I'm not sure that we shouldn't also have the UI set update_only based on the value of addNewBindings, since that's been a point of confusion for users. And I don't think it'd hurt anything. But I'm thinking that the original idea seems like it would result in a better UX. WDYT?

@dyaffe
Copy link
Member

dyaffe commented Oct 7, 2024

I think my expectations on how discovers.update_only works weren't in line with reality. Here was my expectation:

  1. User has automatically add new collections set to false and the UI uses discovers.update_only accordingly during a refresh
  2. We update their collections and also add new bindings as disabled
  3. The user enables new things that they care about

I'm guessing that as part of (2), we don't actually add new bindings as disabled. If that's the case, I think my suggestion would be to update that behavior. Would that make sense?

@psFried
Copy link
Member Author

psFried commented Oct 7, 2024

Yeah, I think that all makes sense. @travjenkins @kiahna-tucker FYI. We might want to break out 1 into a separate front-end issue. Basically to use the value of autoDiscovers.addNewCollections to set the update_only column on the discovers row. Does that make sense to y'all? I'm seeing that as a separable issue.

@travjenkins
Copy link
Member

Sounds good - yeah please just open up a UI issue for that.

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