Update purge suffix so that resources are preserved for at least as long as the configured timeout.#2265
Merged
Update purge suffix so that resources are preserved for at least as long as the configured timeout.#2265
Conversation
…g as the requested timeout. Prior to these changes the effective timeout could be as short as 0:15min instead of the intended 1:15.
asnare
added
bug/test-infra
asnare
changed the title
nfx
requested changes
Jul 29, 2024
| from databricks.labs.ucx.installer.logs import PartialLogRecord, parse_logs | ||
| from databricks.labs.ucx.installer.mixins import InstallationMixin | ||
|
|
||
| from ..mixins.fixtures import get_test_purge_time |
Collaborator
There was a problem hiding this comment.
this includes pytest into the runtime path of UCX installation, which is going to break customers. copy the code. otherwise adjust the new_installation fixture, which is a more proper way of doing this.
This avoids pytest being imported as a (transitive) dependency.
|
✅ 24/24 passed, 1 flaky, 49m31s total Flaky tests:
Running from acceptance #4950 |
JCZuurmond
approved these changes
Jul 30, 2024
| now = datetime.now(timezone.utc) | ||
| purge_deadline = now + timeout | ||
| # Round UP to the next hour boundary: that is when resources will be deleted. | ||
| purge_hour = purge_deadline + (datetime.min.replace(tzinfo=timezone.utc) - purge_deadline) % timedelta(hours=1) |
Contributor
There was a problem hiding this comment.
I find the following logic complicated to round up to an hour: (datetime.min.replace(tzinfo=timezone.utc) - purge_deadline) % timedelta(hours=1)
nfx
approved these changes
Jul 30, 2024
Collaborator
nfx
left a comment
nfx
left a comment
There was a problem hiding this comment.
this is still a hack but okay
nfx
pushed a commit
that referenced
this pull request
Jul 31, 2024
## Changes This PR fixes some tests that check the value of `RemoveAfter` tags: they check how far into the future the tag-value is, and this can now be up to 2 hours due to rounding upwards introduced in #2265. ### Linked issues @ericvergnaud observed some failing acceptance tests on #2330, which this will fix. Resolves #2287. Resolves #2288. Resolves #2289. Resolves #2290. ### Tests - updated integration tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
This PR updates the purge suffix used by integration tests so that resources are marked for preservation at least as long as the configured timeout. The situation is that:
2024072915. This means that resources can be deleted at any moment after 3 p.m. (UTC).2024072915meaning deletion as of 3 p.m. This is 0:15 later instead of the intended 1:15.The default timeout for tests was 1:15 in an attempt to deal with resources being deleted when the test started near the hour boundary. This PR also updates the default timeout to 1:00 because the extra 0:15 isn't needed to ensure this.