Skip to content
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.

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

commonids: add commonid for TenantId #216

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

commonids: add commonid for TenantId #216

wants to merge 3 commits into from

Conversation

catriona-m
Copy link
Member

No description provided.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@tombuildsstuff
Copy link
Contributor

@manicminer given the default Tenant ID can be represented as / - do we also want a DefaultTenantID perhaps?

Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

Agreed, for convenience it might be worth declaring a default tenant for easy re-use. Otherwise LGTM 👍

var DefaultTenantId = TenantId{
	TenantId: "/",
}

Comment on lines +15 to +28
// TenantId is a struct representing the Resource ID for a Tenant
type TenantId struct {
TenantId string
}

// DefaultTenantId returns a default TenantId
var DefaultTenantId = NewTenantID("/")

// NewTenantID returns a new TenantId struct
func NewTenantID(tenantId string) TenantId {
return TenantId{
TenantId: tenantId,
}
}
Copy link
Contributor

@tombuildsstuff tombuildsstuff Mar 15, 2024

Choose a reason for hiding this comment

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

Thinking more about the default Tenant ID perhaps we want to handle this differently?

If we make the TenantId field a pointer, we could support parsing both the Default Tenant ID and another Tenant ID? It'd be different to the other Resource ID parsers but this is a unique case where the Default is handled differently, which I don't believe we've got anywhere else - so perhaps it's worth handling that explicitly here?

Suggested change
// TenantId is a struct representing the Resource ID for a Tenant
type TenantId struct {
TenantId string
}
// DefaultTenantId returns a default TenantId
var DefaultTenantId = NewTenantID("/")
// NewTenantID returns a new TenantId struct
func NewTenantID(tenantId string) TenantId {
return TenantId{
TenantId: tenantId,
}
}
// TenantId is a struct representing the Resource ID for a Tenant
type TenantId struct {
TenantId *string
}
// NewDefaultTenantId returns a TenantId for the default Tenant
func NewDefaultTenantID() TenantId {
return TenantId{}
}
// NewTenantID returns a new TenantId struct
func NewTenantID(tenantId string) TenantId {
return TenantId{
TenantId: &tenantId,
}
}

WDYT @catriona-m @manicminer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants