Add x/config/cli

[nolag]

No description provided.

[github-actions]

📊 API Diff Results

No changes detected for module github.com/smartcontractkit/chainlink-common

View full report

[nolag]

Copilot review says it's done, but didn't post here. It left two comments. The first was because Copilot didn't know about generic methods in Go (get with the times, 1.27 released on Aug 19th changed that).

Second was

{
    "start_line": 214,
    "end_line": 229,
    "fixed": false,
    "fix_skip_reason": "requires_broader_context",
    "fix_skip_note": "The supplied-field bookkeeping needs to cover both pointer fields and their dereferenced values."
  }

Claude says it's wrong, and we have a test to prove it. I agree with Claude.

The comment is wrong, so there's nothing to fix.

• What it claims: validator passes the pointer field to isSupplied, but decode records the address after dereferencing, so the two never match.
• Why that's wrong: validator dereferences a non-nil pointer before it calls a custom rule. fl.Field() is therefore the int the pointer points to, the same value decode records, and the addresses match.
• The nil case: with a nil pointer and nothing supplied, set sees the nil pointer, finds it can't be addressed, and returns false. That's correct: unset.
• The test agrees: the pointer subtest in TestSetIgnoresADefault passes all four cases (pointer set by default or nil, each with and without --value 5). If the comment were right, both --value 5 cases would fail.

The reviewer probably assumed Field() returns the raw struct field. It didn't run the test, which is why it skipped with "requires_broader_context".

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[cedric-cordenier]

Nit: this doesn't need to be generic since it doesn't return anything anymore

[nolag]

It's intentional. The function that will create them wants to ensure the input was the same type as the root.

The one I have in mind right now is for profiles. It'll allow you to say things like "this is the default for EVM with chain ID 1, this is for 2 etc". The T ensures that the profile belongs to the config you provided Register.

[nolag]

Profile's signature

func Profile[T any, K comparable, S any](selector func(*T) *K, profiles map[K]S) RegisterOption[T]

an example use is here

Without the generics returned, you could use profile for the wrong struct. It's unlikely, but possible to do. The generics on the returned RegisterOption forces you to start at the config your registering.

It's still possible to do something wrong if you don't navigate the object itself, but that would be much more intentional. It would be caught (and so would others without the safeguard), but it makes most errors compilation instead of runtime this way.

[nolag]

Above is subject to some change, I'm evaluating an alternative.

[jmank88]

What makes something "wrong" to use though, if that's not backed by the type system?

I'm still digesting the rest, but it seems like targetEntry[T] would connect the dots, and prevent those implementations from having to type cast themselves - did that not work for some reason?

[nolag]

Wrong in my mind is if something can be a complier error but becomes a runtime error. Eg: In this case, I passed a selector function that expects a different type than the main config to be the entry.

I don't get how targetEntry would be generic. Binder holds it, but binder can bind to different types for different commands.

I don't get what you mean by implementations casting it, do you mean specifying T?

[nolag]

Draft for profiles, note I haven't fully reviewed the AI code or comments yet, but the examples seem good enough to demonstrate usage. I may iterate a bit more on the exact API.

This PR is based on a few others that add support for lists and maps as CLI and env vars, the set flag being added, and namespaces. None of them should show up in that PR, so profiles is issolated, but if sourinding code doesn't align with this PR, that's why.

[jmank88]

Can you please document what T is in this context? It reads like an arbitrary additional constraint, and my intuition was apparently way off.