Conversation
Caches the absolute paths of the
ads.yml files associated with a project.
The cache is never used when:
- The list command is invoked
- A command is invoked without a
subcommand AND no services are
defined in the adsroot.yml.
The cache will miss when:
- A removed service is still in the cache.
- The project was switched.
- The services defined in adsroot.yml
are not all present in the cache.
Testing Notes:
- Ran all existing tests and passed.
- Added unit tests and modified existing
integration test to handle caching.
- Performed local testing removing and adding
services. Verified worked as ads without
caching.
|
Not sure what you mean in the PR notes: "The project was switched." |
adamcath
left a comment
adamcath
left a comment
There was a problem hiding this comment.
Strong work! Few things to look into.
| if [[ "$(ls "$project_dir")" ]]; then | ||
| cp -R "$project_dir"/* "$project_tmp" | ||
| fi | ||
| project_cache="$project_tmp/../.ads_cache.yml" |
There was a problem hiding this comment.
This function creates a new temp version of the project. Why would it have a cache that needs to be cleaned?
| self.selectors = selector_set | ||
|
|
||
|
|
||
| ############################################## |
There was a problem hiding this comment.
can you move this to a separate file? the all-one-file thing is an anti-pattern that I've been meaning to move away from. It would be nice to not dig that whole deeper.
| svc_name_to_file[basename] = f | ||
| file_to_svc_name[f] = basename | ||
| return file_to_svc_name | ||
| warning("Duplicate service: %s! Using %s." % |
There was a problem hiding this comment.
Good improvement. Can you print the absolute paths of both files so the user really knows what to do?
| class Project: | ||
| @classmethod | ||
| def load_from_dir(cls, root_dir): | ||
| def load_from_dir(cls, root_dir, profile_dir, check_cache): |
There was a problem hiding this comment.
Project and Profile used to be orthogonal concepts with no dependency on each other. This change muddies the dependency graph. Is that needed? Or, could you compute the Cache first and pass that into the Project?
There was a problem hiding this comment.
Alternatively, store the cache in the project dir instead of the profile? You probably want that anyway, since two different ads projects could have services with the same name.
| spec.get("default"), project_yml) or "all" | ||
|
|
||
| cache = Cache(project_yml, profile_dir) | ||
| if check_cache and cache.cache_map and cache.valid_groups(service_sets): |
| @classmethod | ||
| def get_cache_path(cls, dir_): | ||
| adscache = ".ads_cache.yml" | ||
| cache_home = os.getenv("ADS_CACHE_HOME") |
| adscache = ".ads_cache.yml" | ||
| cache_home = os.getenv("ADS_CACHE_HOME") | ||
| if cache_home: | ||
| return "%s/%s" % (cache_home, adscache) |
There was a problem hiding this comment.
Use os.path.join() to get correct path joining behavior on all platforms.
| if os.path.isfile(cachefile): | ||
| cache_spec = _load_spec_file(cachefile) | ||
|
|
||
| if cache_spec.get(ADS_ROOT) == project_file: |
| not_cached.append(service) | ||
|
|
||
| if not_cached: | ||
| warning("The following service(s) are not cached: " |
There was a problem hiding this comment.
I don't get it. What did the user do wrong, and what do they need to do to fix it?
|
|
||
| def write(self, s): pass | ||
|
|
||
| class TestCache(unittest.TestCase): |
There was a problem hiding this comment.
I'm happy to be convinced that I'm wrong, but I find these unit tests to be pretty weird and confusing, and they don't convince me that cacheing works properly. What do you think of moving some/most of these to being bash tests?
2 participants
Caches the absolute paths of the
ads.yml files associated with a project.
The cache is never used when:
subcommand AND no services are
defined in the adsroot.yml.
The cache will miss when:
are not all present in the cache.
Testing Notes:
integration test to handle caching.
services. Verified worked as ads without
caching.