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

test: Makefile target, CI and documentation for alert unit tests #1355

Merged
merged 12 commits into from
Nov 22, 2024
Merged

test: Makefile target, CI and documentation for alert unit tests #1355

merged 12 commits into from
Nov 22, 2024

Conversation

biswassri
Copy link
Contributor

@biswassri biswassri commented Nov 7, 2024 •
edited
Loading

JIRA: RHOAIENG-8519

Description

Adding make targets for testing prometheus unit tests and for checking alerts that doesn't have any unit tests created. Additionally added information to Readme about the same.

How Has This Been Tested?

make test-alerts

make check-unit-tests

Screenshot or short clip

Merge criteria

  • You have read the contributors guide.
  • Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
  • Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Nov 7, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from biswassri. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 7, 2024

This PR can't be merged just yet 😢

Please run make generate manifests api-docs and commit the changes.

For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/11725694941

@biswassri biswassri mentioned this pull request Nov 7, 2024
Closed
5 tasks
@zdtsw zdtsw requested review from grdryn and StevenTobin November 7, 2024 15:12
ykaliuta
ykaliuta reviewed Nov 7, 2024
View reviewed changes
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
tests/scripts/extract_alerts.sh Outdated Show resolved Hide resolved
tests/scripts/extract_alerts.sh Outdated Show resolved Hide resolved
@ykaliuta
Copy link
Contributor

ykaliuta commented Nov 7, 2024

the commit message (not PR description) does not reflect the changes pretty much.

@biswassri
Copy link
Contributor Author

the commit message (not PR description) does not reflect the changes pretty much.

I was actually squashing commits+cherry pick the squashed commit from my previous PR. I got it wrong a couple of times so I didn't realized the commit message. Sorry! Let me know if there is some way I can fix it ?

@StevenTobin
Copy link
Contributor

the commit message (not PR description) does not reflect the changes pretty much.

I was actually squashing commits+cherry pick the squashed commit from my previous PR. I got it wrong a couple of times so I didn't realized the commit message. Sorry! Let me know if there is some way I can fix it ?

You could use git commit --amend to change the commit message

@biswassri
Copy link
Contributor Author

@zdtsw @StevenTobin @grdryn I'm copying over the comments from the #366 and updating the changes requested there.

biswassri
biswassri commented Nov 8, 2024
View reviewed changes
Makefile Outdated Show resolved Hide resolved
biswassri
biswassri commented Nov 8, 2024
View reviewed changes
Makefile Outdated Show resolved Hide resolved
biswassri
biswassri commented Nov 8, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
@grdryn
Copy link
Member

grdryn commented Nov 12, 2024

/retest

@zdtsw zdtsw requested review from grdryn and ykaliuta November 15, 2024 05:32
ykaliuta
ykaliuta reviewed Nov 15, 2024
View reviewed changes
Makefile Show resolved Hide resolved
ykaliuta
ykaliuta reviewed Nov 15, 2024
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@ykaliuta
Copy link
Contributor

@zdtsw I added a couple of comments but in general fine for me.

@zdtsw
Copy link
Member

zdtsw commented Nov 15, 2024

@zdtsw I added a couple of comments but in general fine for me.

thanks!
I leave @biswassri to sort these out, also wait for @grdryn and @StevenTobin 's review

@biswassri
Copy link
Contributor Author

@ykaliuta Updated the changes from the last comments. @zdtsw Let me know if you have any thoughts and concerns too.

Thanks for the reviews 👍🏻

@zdtsw
Copy link
Member

zdtsw commented Nov 15, 2024

@ykaliuta Updated the changes from the last comments. @zdtsw Let me know if you have any thoughts and concerns too.

Thanks for the reviews 👍🏻

i am fine with the changes.

grdryn
grdryn reviewed Nov 15, 2024
View reviewed changes
Copy link
Member

@grdryn grdryn left a comment

Choose a reason for hiding this comment

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

/lgtm

I made some changes here, so leaving approval for someone else.

Makefile Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the lgtm label Nov 15, 2024
ykaliuta
ykaliuta reviewed Nov 16, 2024
View reviewed changes
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@zdtsw zdtsw removed the rhoai-2.16 label Nov 18, 2024
@openshift-ci openshift-ci bot removed the lgtm label Nov 18, 2024
biswassri and others added 12 commits November 18, 2024 13:32
@biswassri
Adding makefile and scripts for prometheus unit-tests
a507789
@biswassri
Changes from PR review
51e7bea
@biswassri
Update prometheus-params
d44590d
@biswassri
Change extract-alerts to associative array + githubCI fix
ccfde32
@biswassri
Update to github CI
9ba50be
@biswassri
Update check_alert_tests script to use ALERT_SEVERITY
26edb47
@biswassri
Update make test-alert command and remove debug echos
5b5cd68
@biswassri
Update name for Checkout step github workflow
9eb5450
@grdryn @biswassri
chore: remove need for extract_alerts.sh
d1b2ec6 
Make excels at tracking generated output files from specified input
files (it might be the only thing it's good at).

By having a convention of naming the generated rule files and their
corresponding unit test files based on the file names in the
ConfigMap, then we don't need a separate script to track the
difference in what the files are called inside and outside the
ConfigMap, which would need to be updated each time a new unit test
file is added. With this change, as long as you name your unit test
file in the appropriate way, and there's a corresponding entry in the
ConfigMap, then the rule file should get extracted in the expected
way.
@grdryn @biswassri
chore: fix yq in Makefile (no need to be so PHONY)
8def327
@biswassri
Update to Makefile - var assigment
9c66947
@biswassri
Reverting YQ target change
5145525
grdryn
grdryn reviewed Nov 19, 2024
View reviewed changes
Copy link
Member

@grdryn grdryn left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 19, 2024
@biswassri
Copy link
Contributor Author

@zdtsw @ykaliuta @StevenTobin If there are no objections, are we good to merge this?

@ykaliuta
Copy link
Contributor

@zdtsw @ykaliuta @StevenTobin If there are no objections, are we good to merge this?

ok for me

@StevenTobin
Copy link
Contributor

/approve

@zdtsw zdtsw merged commit c1952d9 into opendatahub-io:rhoai Nov 22, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ykaliuta ykaliuta ykaliuta left review comments

@grdryn grdryn grdryn left review comments

@jackdelahunt jackdelahunt Awaiting requested review from jackdelahunt

@Sara4994 Sara4994 Awaiting requested review from Sara4994

@StevenTobin StevenTobin Awaiting requested review from StevenTobin

Assignees

@grdryn grdryn

Labels
lgtm rhoai
Projects
ODH Platform Planning
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@biswassri @ykaliuta @StevenTobin @grdryn @zdtsw