Namespace config #161

jhl-aa · 2024-12-23T02:55:34Z

No description provided.

benbrandt

Since we're bringing in breaking changes, I think we should fix a few more things before merging.

From the user side we are just making it possible to configure more things, but I don't want to change it again after.

I think we have an issue where registry is used in two levels, which hints at a naming problem in our domain.

And more importantly, we need to define what a NamespaceIdentifier is and make sure it is serialized as such from env variables and strings.

I would suggest:

letters
numbers
-

You should be able to check the characters (maybe only allow alphanumeric and - or _ ?) and also use the heck crate if you need to convert. (Though with the previous rules maybe it is as simple as replacing _ with -)

benbrandt · 2024-12-23T04:44:11Z

helm-charts/pharia-kernel/values.yaml

+# Each of the value can alternatively be provided as environment variables, which have higher precedence:
+# NAMESPACES__PLAYGROUND__CONFIG_URL
+# NAMESPACES__PLAYGROUND__CONFIG_ACCESS_TOKEN
+# NAMESPACES__PLAYGROUND__REGISTRY__REGISTRY


I still feel this is telling us we have a bad name here... Shouldn't have registry twice

Changing the field to name

operator-config.toml

src/config.rs

src/namespace_watcher/config.rs

benbrandt · 2024-12-23T04:56:37Z

src/namespace_watcher/config.rs

        config_url: String,
-        config_access_token_env_var: Option<String>,
+        config_access_token: Option<String>,
        registry: Registry,


Ok this is the issue for me. This causes the double registry in the variable which is a signal to me we got the naming wrong.

This isn't always a registry.... I wonder if this is something like source or ... I don't know. But we need another name because then this type has a name that is the same

benbrandt · 2024-12-23T05:21:28Z

On second thought... The normalization would also apply at the url level, so it's possible we can sort this out afterwards... But might still be good to sort out before releasing as a separate breaking release.

benbrandt

We're getting there! Nice work. Still some small things

benbrandt · 2024-12-24T05:18:08Z

operator-config.toml

 [namespaces.pharia-kernel-team.registry]
 type = "oci"
-registry = "registry.gitlab.aleph-alpha.de"
+name = "registry.gitlab.aleph-alpha.de"


I think we're getting closer here. But I still feel like registry is the right name at this level. According to the spec https://github.com/opencontainers/distribution-spec/blob/main/spec.md this is indeed the name of this URL.

I feel like our domain model is slightly off for this enum. The enum is called Registry, but then the variants are a file based or an OCI based one. Which is why i wonder if we need a different name at the top level.

You could argue however, that while this is our concept internally, from a user perspective they only ever deal with OCI registries and therefore registry is the right term at the outer level, which is fair.
In that case I feel this should be domain or host or something.

Sorry to be pedantic about this, but obviously I care we get the right names here :)

Basically I'm sure you guys discussed so:

If you feel the correct name of this domain object is indeed Registry, I am ok with this, but then I wonder if there is a more specific, intuitive name for name

Absolutely, in fact we spent most of the time pondering about this, and we are still not satisfied with the results.

I think it is a symptom of internal (enum) representation leaking into user interface.

pub enum NamespaceConfig { TeamOwned { config_url: String, config_access_token: Option<String>, registry: Registry, }, Watch { directory: PathBuf }, InPlace { skills: Vec<SkillDescription>, registry: Registry, }, }

If we look at the current definition, NamespaceConfig consists of two things:

Skill config

Skill registry (or source)

For TeamOwned, the skill config is already prefixed with config and registry would be prefixed with registry, at least this is consistent.
For Watch, the directory acts as both the skill config and the skill registry.
For InPlace, the skills is the skill config, while the registry is equivalent to TeamOwned.

Thinking top-down, maybe we can streamline the interface:

# TeamOwned + OciRegistry [namespaces.oci-team] config_url = "" config_access_token = "" registry = "" repository = "" registry_user = "" registry_password = "" # TeamOwned + FileRegistry [namespaces.file-team] config_url = "" config_access_token = "" path = "" # Watch [namespaces.dev] directory = "" # InPlace + OciRegistry [namespaces.oci-default] skills = [] registry = "" repository = "" registry_user = "" registry_password = "" # InPlace + FileRegistry [namespaces.file-default] skills = [] path = ""

What do you think?
This would mean flattening the registry, untagging the Registry enum, and adding prefix to user/password.

I'm also considering refactoring config_url and config_access_token as skill_config (which can be via file or http) and skill_config_access_token.

src/config.rs

src/namespace_watcher/config.rs

benbrandt · 2024-12-24T05:21:24Z

src/namespace_watcher/config.rs

-    pub fn from_toml(config: &str) -> anyhow::Result<Self> {
-        Ok(toml::from_str(config)?)
+    fn environment() -> Environment {
+        // A namespace can contain the characters `[a-z0-9-]` e.g. `pharia-kernel-team`.


This is a great doc comment ☺️ just move up one line

benbrandt · 2024-12-24T05:22:24Z

src/namespace_watcher/config.rs

    /// implies that the skills in team owned namespaces are configured by a team rather than the
    /// operators of Pharia Kernel, which in turn means we only refer the teams documentation here.
    TeamOwned {
+        #[serde(alias = "config-url")]


I think we can remove these aliases once we sort out if we need kebab for all environment variables

benbrandt · 2024-12-24T05:23:45Z

src/shell.rs

    /// The qualified name of the skill to invoke. The qualified name consists of a namespace and
    /// a skill name (e.g. "acme/summarize").
-    /// If the namespace is omitted, the default 'pharia-kernel-team' namespace is used.
+    /// If the namespace is omitted, the default `pharia-kernel-team` namespace is used.


Oh we should remove this line. No longer true. It is required

Previously, the namespace config access token, as well as the user and the password for the skill registry are provided as the associated environment variable keys only. As we have now adopted config-rs, the actual values can be provided directly, or more commonly via the associated environment variables.

…r via env var

…ontent

this enforces consistency of configuration names and optimizes the config interface for defining a remote registry

benbrandt suggested changes Dec 23, 2024

View reviewed changes

jhl-aa force-pushed the config-rs branch from 4486184 to 10e9aae Compare December 23, 2024 16:45

benbrandt suggested changes Dec 24, 2024

View reviewed changes

jhl-aa force-pushed the config-rs branch from 9917439 to f84593b Compare January 2, 2025 11:36

moldhouse force-pushed the config-rs branch from a3d16dc to 969fcb8 Compare January 2, 2025 14:48

moldhouse and others added 24 commits January 2, 2025 16:32

test: deserialize registry from environment

d3b9def

test: deserialize nested registry

1d5c8d6

test: deserialize namespace config

0da06ac

test: deserialize operator config from environment

a0f2ead

test: use double underscore as separator

8a1c9a8

test: load config from two sources

7f8b0da

chore: cleanups

9fcd690

test: load two namespaces from independent sources

a84385c

feat: support configuring namespaces via environment variables

6fa7739

refactor: move open telemetry endpoint config next to log level config

13e983a

feat!: support each namespace config fields to be provided as value o…

226ab4c

…r via env var

refactor: consolidate impl blocks for OciRegistry

1bced36

refactor: rename from_sources method

9fb70a0

feat!: rename registry name field as name

1bb1332

test: convert map keys to kebab-case

576f41b

feat: support kebab-case and snake_case for fields in namespace config

235ae45

ci: update environment variables for namespace config

35c9ee9

docs: environment config source

4c080a9

docs: remove outdated comment

f350af7

feat: handle base repository with leading/trailing slashes or empty c…

07ed30b

…ontent

docs: update to new namespace config definition

b965498

feat!: expect operator config variables in kebab-case

2bb5c9e

feat: use untagged enum for registry

995317f

moldhouse and others added 5 commits January 2, 2025 16:32

feat!: flatten repository in operator config

2e30377

this enforces consistency of configuration names and optimizes the config interface for defining a remote registry

feat: prioritize oci registry over file registry

d475a3d

feat: convert namespace name to kebab-case

969dc04

feat: update "assistant" namespace definition to use latest env var

0317159

feat: remove fallback to "dev" namespace for operator-config.toml

0812b28

jhl-aa force-pushed the config-rs branch from acd282b to 0812b28 Compare January 2, 2025 15:32

jhl-aa merged commit e0933df into main Jan 2, 2025
3 checks passed

jhl-aa deleted the config-rs branch January 2, 2025 15:57

Namespace config #161

Namespace config #161

Uh oh!

Conversation

jhl-aa commented Dec 23, 2024

Uh oh!

benbrandt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benbrandt commented Dec 23, 2024

Uh oh!

benbrandt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants