Add notebook options to configs proto #1685
Conversation
|
Will add this to ProjectConfig in core.proto while I'm here |
protos/configs.proto
Outdated
| @@ -40,6 +40,16 @@ message WorkflowSettings { | |||
|
|
|||
| // Optional. The prefix to append to all action names. | |||
| string name_prefix = 9; | |||
|
|
|||
| // Optional. Options for Notebook actions. | |||
| NotebookOptions notebook_options = 10; | |||
There was a problem hiding this comment.
IMO I think we should not nest this. We don't nest anything else in here, even though they logically group (e.g dataset, location). Makes the API cleaner, we don't have a clear plan for what else would go in here either.
defaultNotebookOutputBucket at top level seems fine to me
There was a problem hiding this comment.
Me and Ben were talking about this, we want to migrate to nest the BQ options too
There was a problem hiding this comment.
I'm not against nesting, but I don't think we are ready to design it yet. Agree we should restructure all this, but we need to sit down and think more deeply about how we want to structure these protos long term. Small questions like are these "Notebook" settings or "Vertex" settings for example, or would other actions potentially use GCS buckets that we want to configure (external tables, for example).
Which is why I'd suggest we do the simplest thing for the time being - that doesn't stop us from doing this in a future version!
There was a problem hiding this comment.
If we're following the same pattern as before, Notebook is an abstracted action type that sits across APIs - Vertex settings would break this pattern.
I've updated this to be the flat field because I'm under time pressure to get this in, but when we inevitably make these fields nested, it will be a lot more work to migrate them than if we just do it now!
Done. |
…-proto Add notebook options to configs proto
No description provided.