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.

Sign up for GitHub

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

Jump to bottom

Add validations for the UpdateResultsAPI objects #1057

Merged
merged 7 commits into from
Jan 30, 2024
Merged

Add validations for the UpdateResultsAPI objects #1057

merged 7 commits into from
Jan 30, 2024

Conversation

khansaad
Copy link
Contributor

This PR fixes the validation cases which were left as part of #561

@khansaad khansaad added the bug Something isn't working label Nov 23, 2023
@khansaad khansaad added this to the Kruize 0.0.20.2_rm Release milestone Nov 23, 2023
@khansaad khansaad self-assigned this Nov 23, 2023
@khansaad khansaad linked an issue Nov 23, 2023 that may be closed by this pull request
Validate blank/null/invalid values for keys in updateResults json #561
Closed
@khansaad khansaad marked this pull request as ready for review December 11, 2023 05:01
chandrams
chandrams reviewed Dec 13, 2023
View reviewed changes
tests/scripts/remote_monitoring_tests/rest_apis/test_update_results.py Outdated
@@ -180,9 +180,11 @@ def test_update_results_with_missing_metrics_section(test_name, result_json_file
for err in error_data:
actual_error_message = err["message"]
if test_name == "Missing_metrics_bulk_res_few_containers" and d["interval_end_time"] == "2023-04-13T23:29:20.982Z":
error_message = "kubernetes_objects : Metric data is not present for container : tfb-server-1 for experiment: quarkus-resteasy-kruize-min-http-response-time-db. Metric data is not present for container : tfb-server-0 for experiment: quarkus-resteasy-kruize-min-http-response-time-db"
error_message = "Performance profile: [Metric data is not present for container : tfb-server-1 for experiment: quarkus-resteasy-kruize-min-http-response-time-db. ] , " \
"Performance profile: [Metric data is not present for container : tfb-server-1 for experiment: quarkus-resteasy-kruize-min-http-response-time-db. , " \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is tfb-server-0 changed to tfb-server-1 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted now. It wasn't required.

chandrams
chandrams reviewed Dec 13, 2023
View reviewed changes
tests/scripts/remote_monitoring_tests/helpers/utils.py
@@ -189,8 +189,6 @@ def generate_test_data(csvfile, test_data):

test_name = t + "_" + key
status_code = 400
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to include the below code back and pass createExperiment or updateResults as a parameter to generate_test_data and include the below code for createExp

 if test_name == "invalid_experiment_name" or test_name == "invalid_cluster_name":
                    status_code = 201

Copy link
Contributor

Choose a reason for hiding this comment

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

Also include validation for the expected failure messages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to include the below code back and pass createExperiment or updateResults as a parameter to generate_test_data and include the below code for createExp

 if test_name == "invalid_experiment_name" or test_name == "invalid_cluster_name":
                    status_code = 201

This is done. Validation for the failure messages will be taken up in a different PR.

@msvinaykumar
Copy link
Contributor

@khansaad Please mention list of validations and errors on MD file

@chandrams
Copy link
Contributor

@khansaad - Have you updated the API doc with the error messages?

chandrams
chandrams approved these changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@chandrams chandrams 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
Copy link
Contributor

dinogun commented Jan 23, 2024

@msvinaykumar can you please fix the conflicts here

@msvinaykumar
Copy link
Contributor

@msvinaykumar can you please fix the conflicts here

ok

@msvinaykumar msvinaykumar force-pushed the update-results-api-validation branch from 3c09804 to 83a00ec Compare January 25, 2024 06:46
dinogun
dinogun reviewed Jan 29, 2024
View reviewed changes
...om/autotune/analyzer/serviceObjects/verification/validators/KubernetesElementsValidator.java Outdated
ContainerData kruizeContainer = kruizeContainers.get(i);
ContainerData resultContainer = resultContainers.get(i);
if (!kruizeContainer.getContainer_name().equals(resultContainer.getContainer_name())
|| !kruizeContainer.getContainer_image_name().equals(resultContainer.getContainer_image_name())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest that we skip the image name check for now. If the container image name has changed owing to a application upgrade, our code needs to keep track of that. We cannot penalize the updateResults API for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@khansaad khansaad force-pushed the update-results-api-validation branch from b06fdb8 to 258b0bf Compare January 30, 2024 06:39
@khansaad
fix conflicts
0869ce7 
Signed-off-by: Saad Khan <[email protected]>
dinogun
dinogun approved these changes Jan 30, 2024
View reviewed changes
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 fc881e5 into kruize:mvp_demo Jan 30, 2024
10 of 11 checks passed
msvinaykumar
msvinaykumar reviewed Feb 5, 2024
View reviewed changes
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

@khansaad khansaad mentioned this pull request May 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msvinaykumar msvinaykumar msvinaykumar approved these changes

@chandrams chandrams chandrams approved these changes

@dinogun dinogun dinogun approved these changes

Assignees

@khansaad khansaad

Labels
bug Something isn't working
Projects
Monitoring
Status: Done
Milestone
Kruize 0.0.20.2_rm Release
Development

Successfully merging this pull request may close these issues.

Validate blank/null/invalid values for keys in updateResults json
4 participants
@khansaad @msvinaykumar @chandrams @dinogun