- Notifications
You must be signed in to change notification settings - Fork928
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
RuntimeEntry has no startup value, and omits functions requiredto 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 ifcache 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 tomain. 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 aboutsyncmap
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
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Danny Kopping <danny@coder.com>
23ef17e intodk/runtime-configsUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
DeploymentEntryhas noNew()function because it is always instantiated from aserpentcontext. There is no point to implementing aNew()function that will never be used in prod.Managerthat will be instantiated once for the app. TheManagerproducesResolverssuch that we can implement some shared cache.