-
Notifications
You must be signed in to change notification settings - Fork 188
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
fix: Set last_update_date
to the time reported by the REST endpoint
#2672
Conversation
docker/importer/importer_test.py
Outdated
imp = importer.Importer('fake_public_key', 'fake_private_key', self.tmp_dir, | ||
importer.DEFAULT_PUBLIC_LOGGING_BUCKET, 'bucket', | ||
False, False) | ||
imp.run() | ||
self.assertEqual(mock_publish.call_count, data_handler.cve_count) | ||
self.assertEqual(repo.get().last_update_date, datetime.datetime(2024, 1, 1)) |
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.
Could you add a msg
attribute to this with a human-friendly string explaining the nature of the assertion failure to help with debugging test failures?
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.
Added in 9ce7a86
logging.info('No changes since last update.') | ||
return | ||
request_last_modified = None | ||
if last_modified := request.headers.get('Last-Modified'): |
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.
docker/importer/importer.py
Outdated
adapter = HTTPAdapter( | ||
max_retries=Retry( | ||
total=3, status_forcelist=[502, 503, 504], backoff_factor=1)) | ||
s.mount(source_repo.rest_api_url, adapter) |
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.
Rather than mounting these explicitly, would it make sense (and is it possible to) just do a catchall here to apply this to all URLs?
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.
Worked out how to catchall by mounting on http://
and https://
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
@@ -821,7 +825,7 @@ def setUp(self): | |||
self.tmp_dir = tempfile.mkdtemp() | |||
|
|||
tests.mock_datetime(self) | |||
warnings.filterwarnings("ignore", "unclosed", ResourceWarning) | |||
warnings.filterwarnings('ignore', category=SystemTimeWarning) |
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 didn't realize there was a way to squelch these, this is awesome! Thank you!!!
Use the
Last-Modified
header (or the latestmodified
vuln date if the header is missing) to set the REST SourceRepository'slast_update_date
, instead of using the time when the importer ran.Should alleviate #2670 for well-behaved REST sources, though it still doesn't handle cases where the source itself adds a record with date before the previous updated date.
While I was here, I also tidied up the
ignore_last_import_time
logic a bit (because the long line splitting was annoying).