-
Notifications
You must be signed in to change notification settings - Fork 886
chore: refactor entry into deployment and runtime #14575
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
Conversation
RuntimeEntry has no startup value, and omits functions required to be serpent compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
} | ||
|
||
func NewMemoryCachedResolver(cache *syncmap.Map[string, cacheEntry], wrapped Resolver) *MemoryCachedResolver { | ||
return &MemoryCachedResolver{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: panic here if cache
is nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should actually delete the cache resolver before we merge to main
. Was just testing out some api usage with it.
// Validate that it returns that value. | ||
require.Equal(t, base.String(), field.String()) | ||
// Validate that there is no org-level override right now. | ||
_, err := field.Resolve(ctx, mgr.DeploymentResolver(db)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: DeploymentResolver
feels like the wrong name; let's keep it for now but it's worth revisiting, I feel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
// resolver from the manager. | ||
// TODO: Delete MemoryCacheManager and implement it properly in 'StoreManager'. | ||
type MemoryCacheManager struct { | ||
cache *syncmap.Map[string, cacheEntry] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! TIL about syncmap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handy little guy
Co-authored-by: Danny Kopping <danny@coder.com>
DeploymentEntry
has noNew()
function because it is always instantiated from aserpent
context. There is no point to implementing aNew()
function that will never be used in prod.Manager
that will be instantiated once for the app. TheManager
producesResolvers
such that we can implement some shared cache.