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

Support 'clear_cache' #5266

Merged
merged 2 commits into from Mar 16, 2021
Merged

Support 'clear_cache' #5266

merged 2 commits into from Mar 16, 2021

Conversation

Schwad
Copy link
Contributor

@Schwad Schwad commented Sep 29, 2020

Helps with Customer Bug 236 (see there for repro)

Currently, when we run inspec compliance upload my_profile it is
cached locally in inspec when run. If we update the version in the core
code and run another upload, inspec compliance upload my_profile again
it will run the old cached version instead of running a new copy
from automate.

The current workaround is to specify the desired version with
inspec exec compliance://my_profile/admin#0.1.1.

The caching happens before we have forward sight into the profile's
contents and only the target name. So the text used to generate the
cache would be compliance://my_profile/admin which does not change
version to version.

A fix here could simply identify when we are doing a local inspec exec compliance:// (hitting local profiles does not generate a cache) and
skips the cache if there's no version specified. That would eliminate
the unexpected behavior. However, it is a breaking change for customers
as some current caching taking place would no longer take place.

Instead, we have included a clear_cache cli method for InSpec,
which should assist the core team and other developers in the future
when debugging edge case issues in InSpec.

Signed-off-by: Nick Schwaderer nschwaderer@chef.io

< edited 2020-11-24 >

@Schwad Schwad requested a review from a team as a code owner September 29, 2020 12:27
@netlify
Copy link

netlify bot commented Sep 29, 2020

Deploy preview for chef-inspec canceled.

Built with commit cbe105b

https://app.netlify.com/sites/chef-inspec/deploys/60510a14fb9d070008f0cbd3

@Schwad
Copy link
Contributor Author

Schwad commented Sep 29, 2020

Checking CI builds here. Still need to implement tests for cache buster and ensuring our case is covered with the caching.

lib/inspec/cached_fetcher.rb Outdated Show resolved Hide resolved
lib/inspec/cached_fetcher.rb Outdated Show resolved Hide resolved
lib/inspec/cli.rb Outdated Show resolved Hide resolved
lib/inspec/cli.rb Outdated Show resolved Hide resolved
@james-stocks
Copy link
Contributor

Big 👍 from me on making this change; but I think we should run it past @kekaichinose - some existing customers who are currently happy to run with a cached profile every time might perceive this change as a reduction in performance and increase in traffic. We'd just need to be ready to explain the improvement here.

@Schwad Schwad force-pushed the ns/cust_issue branch 2 times, most recently from 7fc5caa to ce41fe2 Compare October 19, 2020 10:26
@Schwad
Copy link
Contributor Author

Schwad commented Nov 11, 2020

This is currently blocked from the product side due to changing the caching experience for current users whilst solving the problem. I am currently looking into alternative ways to implement the fix without changing the experience. If I fail, then we'll have to simply ship the clear_cache as the option for users to debug this.

@Schwad
Copy link
Contributor Author

Schwad commented Nov 24, 2020

Updated the description to match commit description. It appears there's not a quick and easy way to back door the caching, and the first solution has not been accepted. Therefore I have let those that raised the issue know about the situation and have updated this PR just to implement clear_cache which will at least assist folks with rectifying this in the future.

@Schwad Schwad changed the title Do not cache compliance when version unspecified, support 'clear_cache' Support 'clear_cache' Nov 24, 2020
Comment on lines 7 to 11
parallelize_me!

describe "inspec clear_cache" do
it "clears any existing cache" do
dirname = File.expand_path("~/.inspec/cache")
Copy link
Contributor

Choose a reason for hiding this comment

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

Parallelizing and using a global filesystem location during testing seems like asking for trouble. Any other test that caches may be writing to the cache at the same time, and may have it cleared during its testing.

Since the implementation supports using a --vendor-cache option for a custom cache location, it might be safer to use a temporary directory in a block and pass that as the cache location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I only just responded here - thanks for this point and I've updated the PR to hopefully catch this via the vendor-cache path. :)

@Schwad
Copy link
Contributor Author

Schwad commented Jan 22, 2021

Ahoy! 👋 just dropping in to say I've had a look at this and it seems it's red because of two failures in the tests (likely something syntax-y).

I'm hopeful I'll get a change to pull this up on my machine and get things green on this PR! 👍

I can't see artifact and hab-build (rightfully so as they're blacklisted from community viewing) but I'm assuming those reds might be something a bit flaky or unrelated to the PR. 🤔

Nick Schwaderer added 2 commits March 16, 2021 15:41
Helps with Customer Bug 236 (see there for repro)

Currently, when we run `inspec compliance upload my_profile` it is
cached locally in inspec when run. If we update the version in the core
code and run another upload, `inspec compliance upload my_profile` again
it will run the old cached version instead of running a new copy
from automate.

The current workaround is to specify the desired version with
`inspec exec compliance://my_profile/admin#0.1.1`.

The caching happens before we have forward sight into the profile's
contents and only the target name. So the text used to generate the
cache would be `compliance://my_profile/admin` which does not change
version to version.

A fix here could simply identify when we are doing a local `inspec exec
compliance://` (hitting local profiles does not generate a cache) and
skips the cache if there's no version specified. That would eliminate
the unexpected behavior. However, it is a breaking change for customers
as some current caching taking place would no longer take place.

Instead, we have included a `clear_cache` cli method for InSpec,
which should assist the core team and other developers in the future
when debugging edge case issues in InSpec.

Signed-off-by: Nick Schwaderer <nschwaderer@chef.io>
Signed-off-by: Nick Schwaderer <nschwaderer@chef.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants