feat: Implement appconfig package #1625
Conversation
jachym-tousek-keboola
force-pushed
the
jt-PSGO-386-sandboxes-service
branch
2 times, most recently
from
9370855
to
90f17ad
Compare
| cacheControl := response.ResponseHeader().Get("Cache-Control") | ||
| if cacheControl == "" { | ||
| return nil | ||
| } | ||
|
|
||
| cacheDirectives, err := cacheobject.ParseResponseCacheControl(cacheControl) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if cacheDirectives.NoStore || cacheDirectives.NoCache != nil { | ||
| return nil | ||
| } | ||
|
|
||
| result.maxAge = time.Second * time.Duration(cacheDirectives.MaxAge) |
There was a problem hiding this comment.
Cache-Control header handling:
- of header is missing or configured to disable caching then don't cache anything
- if MaxAge is set then use it
| type SandboxesError struct { | ||
| Message string `json:"error"` | ||
| ExceptionID string `json:"exceptionId"` | ||
| request *http.Request | ||
| response *http.Response | ||
| } |
There was a problem hiding this comment.
Difference to QueueError: There was a discrepancy between http code and the code in json response. Pepa said I should use the http code so I removed the code that was handling the code from json.
| // An error other than 404 is considered a temporary failure. Keep using the stale cache for staleCacheFallbackDuration as fallback. | ||
| if fallbackItem != nil && now.Before(fallbackItem.expiresAt.Add(staleCacheFallbackDuration)) { | ||
| logger.Warnf(ctx, `Using stale cache for app "%s": %s`, appID, err.Error()) | ||
|
|
||
| return fallbackItem.config, nil | ||
| } |
There was a problem hiding this comment.
Pepa wanted me to use the stale cache configuration on error while refreshing the configuration. In my opinion this should only be the case for temporary errors. This is only for errors of sandboxes api itself, invalid configuration is handled elsewhere.
| type attempt struct { | ||
| delay time.Duration | ||
| responses []*http.Response | ||
| expectedETag string | ||
| expectedErrorCode int | ||
| expectedConfig AppProxyConfig | ||
| } |
There was a problem hiding this comment.
The logic inside Loader is mainly about correct handling of ETag and Cache-Control headers - deciding when to contact the sandboxed api and when to use the cache. So the test cases need to do several attempts to get the config - some attempts are after a delay, others with incorrect etag, then the stale cache usage etc. Writing this test was the most complicated part.
jachym-tousek-keboola
force-pushed
the
jt-PSGO-386-sandboxes-service
branch
from
90f17ad
to
b4e4599
Compare
jachym-tousek-keboola
force-pushed
the
jt-PSGO-386-sandboxes-service
branch
from
b4e4599
to
32482ad
Compare
jachym-tousek-keboola
force-pushed
the
jt-PSGO-386-sandboxes-service
branch
from
ac6817c
to
cdfa7cd
Compare
| type SandboxesError struct { | ||
| Message string `json:"error"` | ||
| ExceptionID string `json:"exceptionId"` | ||
| request *http.Request | ||
| response *http.Response | ||
| } |
| // An error other than 404 is considered a temporary failure. Keep using the stale cache for staleCacheFallbackDuration as fallback. | ||
| if fallbackItem != nil && now.Before(fallbackItem.expiresAt.Add(staleCacheFallbackDuration)) { | ||
| logger.Warnf(ctx, `Using stale cache for app "%s": %s`, appID, err.Error()) | ||
|
|
||
| return fallbackItem.config, nil | ||
| } |
| loader := NewLoader(log.NewDebugLogger(), clk, url) | ||
| loader.sender = loader.sender.(client.Client).WithTransport(transport) |
There was a problem hiding this comment.
Please, inject the client as a dep. via constructor.
What if you need to mock the transport at a higher level?
You can also switch the test to blackbox mode, package ..._test, you will only test the public interface of the Loader, and it will lead to a better design.
There was a problem hiding this comment.
Nice improvement, ty!
There was a problem hiding this comment.
Addressed in #1635
| type attempt struct { | ||
| delay time.Duration | ||
| responses []*http.Response | ||
| expectedETag string | ||
| expectedErrorCode int | ||
| expectedConfig AppProxyConfig | ||
| } |
Jira: https://keboola.atlassian.net/browse/PSGO-386
Changes: