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

Add validation for Update Results API Params #814

Merged
merged 7 commits into from
Nov 2, 2023

Conversation

khansaad
Copy link
Contributor

@khansaad khansaad commented May 9, 2023

This PR fixes #561 . It contains validation for the update results API's input params.

@khansaad khansaad added the bug Something isn't working label May 9, 2023
@khansaad khansaad added this to the Kruize 0.0.15_rm Release milestone May 9, 2023
@khansaad khansaad self-assigned this May 9, 2023
Signed-off-by: saakhan <[email protected]>
@chandrams
Copy link
Contributor

@khansaad - As discussed, please update the expected error messages in the negative functional tests

…-exp-json

# Conflicts:
#	src/main/java/com/autotune/analyzer/serviceObjects/Converters.java
#	src/main/java/com/autotune/analyzer/services/UpdateResults.java
#	src/main/java/com/autotune/analyzer/utils/AnalyzerErrorConstants.java
@khansaad khansaad changed the title Add validation for Agregation info values Add validation for Update Results API Params Nov 2, 2023
Copy link
Contributor

@msvinaykumar msvinaykumar left a comment

Choose a reason for hiding this comment

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

LGTM

public static final String MEASUREMENT_DURATION_ERROR = "Interval duration cannot be less than or greater than measurement_duration by more than " + KruizeConstants.TimeConv.MEASUREMENT_DURATION_THRESHOLD_SECONDS + " seconds";
public static final String MISSING_METRICS = "Metric data is not present for container : %s for experiment: %s. ";
public static final String BLANK_AGGREGATION_INFO_VALUE = " cannot be negative or blank for the metric variable: ";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the tests with the expected error messages validation

@khansaad khansaad added this to the Kruize 0.0.20_rm Release milestone Nov 2, 2023
Copy link
Contributor

@dinogun dinogun left a comment

Choose a reason for hiding this comment

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

LGTM

@dinogun dinogun merged commit e37a447 into kruize:mvp_demo Nov 2, 2023
4 of 5 checks passed
@chandrams
Copy link
Contributor

@khansaad - Please raise a separate PR with the test changes as this PR is closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Validate blank/null/invalid values for keys in updateResults json
5 participants