Skip to content

pkg/contexts: expand CRE.Normalized #1746

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.

Sign up for GitHub

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

Merged
jmank88 merged 1 commit into from
Dec 31, 2025
Merged

pkg/contexts: expand CRE.Normalized #1746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions pkg/contexts/contexts.go
View file
Open in desktop
Copy link
Contributor Author

@jmank88 jmank88 Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bolekk Do you have any concerns with making the others lower-case as well?

Copy link
Contributor

@bolekk bolekk Dec 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No concerns but are you sure this is the best way to handle it? What if somebody misspells "owner_"? It will be silently ignored again. I thought that it might be better to enforce one single format in CLD (and optionally here too).

Copy link
Contributor Author

@jmank88 jmank88 Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed to being stricter on the settings side, but I would worry about having to update this code in the future if it were too strict. Is there a chance that formats could change? Or is that not something to worry about?

Copy link
Contributor Author

@jmank88 jmank88 Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and we don't have an existing error here, so it would be messy to reject things

Copy link
Contributor

@bolekk bolekk Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK so let's be more permissive here but also add validations in CLD.

Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,16 @@ type CRE struct {

// Normalized returns a possibly modified CRE with normalized values.
func (c CRE) Normalized() CRE {
c.Org = strings.TrimPrefix(c.Org, "org_")
c.Org = strings.TrimPrefix(c.Org, "0x")
c.Org = strings.ToLower(c.Org)

c.Owner = strings.TrimPrefix(c.Owner, "owner_")
c.Owner = strings.TrimPrefix(c.Owner, "0x")
c.Owner = strings.ToLower(c.Owner)

c.Workflow = strings.TrimPrefix(c.Workflow, "0x")
c.Workflow = strings.ToLower(c.Workflow)
return c
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/settings/keys_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import (

func TestTenant_rawKeys(t *testing.T) {
const (
org = "AcmeCorporation"
org = "acmecorporation"
owner = "1234abcd"
workflow = "ABCDEFGH"
workflow = "abcdefgh"
key = "foo"
)
for _, test := range []struct {
Expand Down
6 changes: 3 additions & 3 deletions pkg/settings/limits/resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func ExampleResourceLimiter_Use() {
func ExampleMultiResourcePoolLimiter() {
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()
ctx = contexts.WithCRE(ctx, contexts.CRE{Org: "orgID", Owner: "owner-id", Workflow: "workflowID"})
ctx = contexts.WithCRE(ctx, contexts.CRE{Org: "org-id", Owner: "owner-id", Workflow: "workflow-id"})
global := GlobalResourcePoolLimiter[int](100)
freeGlobal, err := global.Wait(ctx, 95)
if err != nil {
Expand Down Expand Up @@ -118,9 +118,9 @@ func ExampleMultiResourcePoolLimiter() {
free()
// Output:
// resource limited: cannot use 10, already using 95/100
// resource limited for org[orgID]: cannot use 10, already using 45/50
// resource limited for org[org-id]: cannot use 10, already using 45/50
// resource limited for owner[owner-id]: cannot use 10, already using 15/20
// resource limited for workflow[workflowID]: cannot use 10, already using 5/10
// resource limited for workflow[workflow-id]: cannot use 10, already using 5/10
// <nil>
}

Expand Down
Loading