-
Notifications
You must be signed in to change notification settings - Fork 79
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
[Fleet] Support download rate in string with unit format #3677
Conversation
"github.com/docker/go-units" | ||
) | ||
|
||
func parseDownloadRate(jsonDownloadRate json.RawMessage) (*float64, error) { |
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.
We should adjust the openapi spec for the download rate to be:
oneOf:
- type: string
- type: float
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.
@michel-laterman I am wondering if we should really change the openAPISpec, as this mostly a temporary fix to support a client that did not sent the correct value.
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.
Don't make the error an officially accepted value. Tolerate it but don't encourage it's use.
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.
OK, can you add a line in the changelog and as a description to this function to indicate this
also can you add tests that just test a random string input, and another that tests "MBps"
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.
Yes I will add one with a random string input already had one with MBps
"github.com/docker/go-units" | ||
) | ||
|
||
func parseDownloadRate(jsonDownloadRate json.RawMessage) (*float64, error) { |
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.
OK, can you add a line in the changelog and as a description to this function to indicate this
also can you add tests that just test a random string input, and another that tests "MBps"
@michel-laterman I think it will make sense to backport this 8.15 no? |
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.
Please backport and add a an additional test case
expectedValue: 1000000.00, | ||
}, { | ||
name: "download rate random string", | ||
raw: json.RawMessage(`"toto"`), |
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.
My suggestion for an additional test case would be to include just the suffix that is checked:
json.RawMessage(`"MBps"`)
as a test case
Quality Gate passedIssues Measures |
(cherry picked from commit b04157a)
Description
Relates https://github.com/elastic/ingest-dev/issues/3446
Elastic-agent are currently not sending download rate as a float but as a string with unit, while this is fixed on the agent side and for older agent, Fleet server should accept both format.
That PR does that, and return a 400 instead of a 500 when an error parsing meta happen.
Todo