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

feat: sumologic timeslice validation [PC-7427] #238

Conversation

nikodemrafalski
Copy link
Contributor

@nikodemrafalski nikodemrafalski commented Jan 12, 2024

Tightens sumo logic log query validation to allow timeslice operator to be used with 15,30 and 60 sec as an argument

@nikodemrafalski nikodemrafalski marked this pull request as draft January 12, 2024 14:46
@nikodemrafalski nikodemrafalski marked this pull request as ready for review January 15, 2024 12:21
@nikodemrafalski nikodemrafalski force-pushed the PC-7427-validation-for-timeslice-keyword-used-in-sumo-logic-logs-based-queries branch from ce5f558 to b41d4de Compare January 16, 2024 08:49
Copy link
Contributor

@bdw-nobl9 bdw-nobl9 left a comment

Choose a reason for hiding this comment

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

Leaving loose ideas and nitpicks, but approved! 🦭

manifest/v1alpha/slo/metrics_sumo_logic.go Outdated Show resolved Hide resolved
manifest/v1alpha/slo/metrics_sumo_logic.go Outdated Show resolved Hide resolved
@nikodemrafalski
Copy link
Contributor Author

nikodemrafalski commented Jan 24, 2024

@bdw-nobl9 @nieomylnieja

I'm not a fan of adding a const just because a particular string is used in code and in tests.
If I introduce a const with an error message and rely on it in tests, someone can change the value of the string in const, and the test will still pass.

I would do it if the value were used twice in the actual system under test, but it's used once in this case.

@bdw-nobl9
Copy link
Contributor

@bartek @nieomylnieja

I'm not a fan of adding a const just because a particular string is used in code and in tests.

If I introduce a const with an error message and rely on it in tests, someone can change the value of the string in const, and the test will still pass.

I would do it if the value were used twice in the actual system under test, but it's used once in this case.

Fine for me (-: especially it would have only a single real usage

@bdw-nobl9
Copy link
Contributor

@bartek @nieomylnieja

I'm not a fan of adding a const just because a particular string is used in code and in tests.

If I introduce a const with an error message and rely on it in tests, someone can change the value of the string in const, and the test will still pass.

I would do it if the value were used twice in the actual system under test, but it's used once in this case.

You mentioned some other Bartek (-;

@n9-machine-user n9-machine-user added enhancement New feature or request go minor labels Jan 25, 2024
@nikodemrafalski nikodemrafalski merged commit ec30bda into main Feb 2, 2024
4 checks passed
@nikodemrafalski nikodemrafalski deleted the PC-7427-validation-for-timeslice-keyword-used-in-sumo-logic-logs-based-queries branch February 2, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request go minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants