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

Extract version utils and use semver for version comparison #4844

Merged
merged 39 commits into from
Oct 31, 2019

Conversation

hithwen
Copy link
Contributor

@hithwen hithwen commented Oct 21, 2019

Use semver for version comparison, extract version_utils
There is another PR after this to submit version metadata

@codecov
Copy link

codecov bot commented Oct 23, 2019

Codecov Report

Merging #4844 into master will decrease coverage by 8.82%.
The diff coverage is 90.36%.

Impacted Files Coverage Δ
postgres/tests/test_integration.py 98.11% <100%> (ø)
postgres/tests/test_unit.py 100% <100%> (ø)
postgres/tests/test_version_utils.py 100% <100%> (ø)
postgres/tests/conftest.py 100% <100%> (ø)
postgres/datadog_checks/postgres/postgres.py 76.07% <66.66%> (ø)
postgres/datadog_checks/postgres/version_utils.py 96.42% <96.42%> (ø)
disk/tests/mocks.py
disk/datadog_checks/disk/__init__.py
disk/tests/conftest.py
disk/tests/test_check.py
... and 100 more

@hithwen hithwen force-pushed the julia/pg-refactor-use-semver branch from 6e6ed59 to 6b3af0d Compare October 23, 2019 10:57
@hithwen hithwen changed the title Julia/pg refactor use semver extract version utils and use semver for version comparison Oct 24, 2019
@hithwen hithwen marked this pull request as ready for review October 24, 2019 10:34
@hithwen hithwen requested review from a team as code owners October 24, 2019 10:34
@hithwen hithwen force-pushed the julia/pg-refactor-use-semver branch from eafc603 to fb0cba8 Compare October 24, 2019 12:10
@hithwen hithwen mentioned this pull request Oct 24, 2019
return semver.VersionInfo(*version)
except Exception:
# Postgres might be in development, with format \d+[beta|rc]\d+
match = re.match(r'(\d+)([a-zA-Z]+)(\d+)', raw_version)
Copy link
Contributor

Choose a reason for hiding this comment

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

is X.YbetaZ possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. PG documentation only refers to actual releases, which are on X.Y.Z format https://www.postgresql.org/support/versioning/

Copy link
Member

Choose a reason for hiding this comment

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

According to https://github.com/postgres/postgres/releases/, postgres seems to follow semver, only betas differ from semver and they seem to only apply to major versions:
For example: postgres/postgres@a240570#diff-e2d5a00791bce9a01f99bc6fd613a39dR585

def _is_10_or_above(self):
return self._is_above([10, 0, 0])
@property
def version(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't cache version information as it may change

Copy link
Contributor Author

@hithwen hithwen Oct 29, 2019

Choose a reason for hiding this comment

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

The cache is cleared when wrong version is detected (see _clean_state). Take into account we are already caching version specific information such as which queries to run

Copy link
Contributor

@ofek ofek Oct 29, 2019

Choose a reason for hiding this comment

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

It's only cleared when an error occurs and the difference between minor releases is unlikely to cause one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that is right and that would affect inventories project only. So I'll remove the caching as part of the PR that adds inventories. This PR is not intending to change behaviour and version was already cached (#4874 (comment))

Copy link
Member

@FlorianVeaux FlorianVeaux left a comment

Choose a reason for hiding this comment

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

A few comments but looks good overall !

postgres/datadog_checks/postgres/postgres.py Show resolved Hide resolved
return semver.VersionInfo(*version)
except Exception:
# Postgres might be in development, with format \d+[beta|rc]\d+
match = re.match(r'(\d+)([a-zA-Z]+)(\d+)', raw_version)
Copy link
Member

Choose a reason for hiding this comment

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

According to https://github.com/postgres/postgres/releases/, postgres seems to follow semver, only betas differ from semver and they seem to only apply to major versions:
For example: postgres/postgres@a240570#diff-e2d5a00791bce9a01f99bc6fd613a39dR585

postgres/datadog_checks/postgres/postgres.py Outdated Show resolved Hide resolved
@hithwen hithwen merged commit c6537d7 into master Oct 31, 2019
@hithwen hithwen deleted the julia/pg-refactor-use-semver branch October 31, 2019 13:28
@AlexandreYang AlexandreYang changed the title extract version utils and use semver for version comparison Extract version utils and use semver for version comparison Dec 2, 2019
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.

4 participants