Support Fluentd config API endpoint for metadata collection #6062
Conversation
…d-metadata-collection
Codecov Report
|
fluentd/tests/util.py
Outdated
| from .common import FLUENTD_VERSION | ||
|
|
||
| requires_1_9 = pytest.mark.skipif( | ||
| FLUENTD_VERSION is None or FLUENTD_VERSION != '1.9.3', |
There was a problem hiding this comment.
Should be >= instead of != ?
That will ease maintenance if we add new versions to tox.ini that also need metadata test or update 1.9.3 to something higher.
There was a problem hiding this comment.
I'm wary of using >= with a string
>>> '1.9.3' > '1.10.3'
True
There was a problem hiding this comment.
You can maybe use this:
>>> from packaging import version
>>> version.parse("1.9.3") >= version.parse("1.10.3")
FalseThere was a problem hiding this comment.
We could even build a pair of @requires_below_1_8_0/@requires_above_1_8_0 since this is the exact pivot version for the things we're testing (we don't actually care that we use 1.9.0 vs 0.12, it's only important that there's a version before 1.8.0 and one after).
There was a problem hiding this comment.
New bunch of nits building on @AlexandreYang's comments :-)
| config = r.json() | ||
| raw_version = config.get('version') | ||
| except Exception as e: | ||
| self.log.debug("No config could be retrieved from %s: %s", self.config_url, e) |
There was a problem hiding this comment.
Nit: we can simplify the logic a bit by applying the fallback here, dropping the need for raw_version = None initially. Maybe we can also embed the minimal fluentd version for background:
| self.log.debug("No config could be retrieved from %s: %s", self.config_url, e) | |
| self.log.debug("No config could be retrieved from %s: %s", self.config_url, e) | |
| # Fall back to command line as this may be an older version of Fluentd (< 1.8.0) | |
| raw_version = self._get_version_from_command_line() |
There was a problem hiding this comment.
raw_version = config.get('version')
Will return None (no exception) if the request to the config endpoint is succesfull but is an older version of fluentd
fluentd/tests/util.py
Outdated
| from .common import FLUENTD_VERSION | ||
|
|
||
| requires_1_9 = pytest.mark.skipif( | ||
| FLUENTD_VERSION is None or FLUENTD_VERSION != '1.9.3', |
There was a problem hiding this comment.
We could even build a pair of @requires_below_1_8_0/@requires_above_1_8_0 since this is the exact pivot version for the things we're testing (we don't actually care that we use 1.9.0 vs 0.12, it's only important that there's a version before 1.8.0 and one after).
florimondmanca
changed the title
|
I went ahead and updated the PR title :-) |
| self.url = self.instance.get('monitor_agent_url') | ||
| parsed_url = urlparse(self.url) | ||
| self.monitor_agent_host = parsed_url.hostname | ||
| self.monitor_agent_port = parsed_url.port or 24220 |
There was a problem hiding this comment.
(nit) Might be good to have the default port as constant DEFAULT_PORT = 24220.
Use updated config rest endpoint for metadata collection and fallback to cli for older versions of fluentd