Restrict teams features based on org status

[wojtekmach]

No description provided.

[github-actions]

Uffizzi Ephemeral Environment deployment-59180

☁️ https://app.uffizzi.com/github.com/livebook-dev/livebook/pull/2897

📄 View Application Logs etc.

What is Uffizzi? Learn more!

[wojtekmach]

OK, we have a dependency issue here. We need to either:

1. Deploy Teams with this before we publish a new Livebook version
2. Rename field to org_disabled : bool or something like org_status : active | disabled

The reason is the default value for booleans is false so if Teams is not sending this value it is considered false which would block Teams features. The DSL allows a default: true, which would be my preference, but that is not available on proto3.

I guess option 1 is fine, option 2 with :org_status could be more future proof though. @jonatanklosko @josevalim @hugobarauna Thoughts?

[josevalim]

Making it an enum is probably going to be better in the long term?

[jonatanklosko]

@josevalim

livebook/proto/messages.proto

Lines 164 to 174 in 04a93c0

	/**
	* We're only using Enum in this case because
	* the Client is the source of the information,
	* and the Server will always be up-to-date.
	*
	* Otherwise, it shouldn't be used.
	*/
	enum AppDeploymentStatusType {
	preparing = 0;
	available = 1;
	}

[jonatanklosko]

We are going to deploy Teams first anyway (1.), otherwise the new Livebook release would probably have issues, no?

[josevalim]

Yes, correct. We always need to deploy Livebook Teams first before we release a new Livebook. But if anyone uses main or nightly build, then it will be fully broken saying they need to pay. That's why we may also want to have an enum with better defaults? Or the enum would be broken as well?

[wojtekmach]

Why couldn't we return more values in the future? Is that Protobuf limitation or it's depend on us how we treat incoming values (e.g. exhaustive case/2 on exact values)?

[josevalim]

So let's ship with the inverse for now: inactive

[jonatanklosko]

Why couldn't we return more values in the future? Is that Protobuf limitation or it's depend on us how we treat incoming values (e.g. exhaustive case/2 on exact values)?

The protobuf library would keep an unknown value, but it would be an integer (see commit message in elixir-protobuf/protobuf@6b8f40f). Leaking the integer to the rest of the application is probably a bad idea, so we would need to account for the unknown value early. In some cases this could be as simple as picking a default value, but here I'm not sure if that's clear. Using strings has the benefit that it's safer to keep them around, but it mostly pushes the catch all and default behaviour problem to other parts of the app.

I don't remember if there was a specific scenario behind the AppDeploymentStatusType comment though.

[wojtekmach]

Im gonna go with inactive bool flag then. Though if anyone prefers can call it disabled, lmk. Thanks everyone!

[jonatanklosko]

Sounds good to me!