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 limit to the rules api #10152

Merged
merged 5 commits into from Jan 12, 2022
Merged

add limit to the rules api #10152

merged 5 commits into from Jan 12, 2022

Conversation

aksharma53
Copy link
Contributor

Trying to add limit and close this issue #9342
thanks @LeviHarrison for review I have incorporated your comments in this pr
please do let me know for other changes if any.

Signed-off-by: alphasaurs alphasaurs@gmail.com

@LeviHarrison
Copy link
Member

Closing and re-opening to trigger CI.

Copy link
Member

@LeviHarrison LeviHarrison left a comment

Choose a reason for hiding this comment

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

Almost there! Thanks for making those changes, and DCO looks good this time. Presuming the tests pass, which I think they will, there's only one more thing: the limit field should be added to the sample API response right below the interval field.

"interval": 60,

@aksharma53
Copy link
Contributor Author

I have added that too do let me know for tasks that needed to be completed before this release would like to learn and contribute more to this project

alphasaurs added 4 commits January 11, 2022 21:50
Signed-off-by: alphasaurs <alphasaurs@gmail.com>
Signed-off-by: alphasaurs <alphasaurs@gmail.com>
This reverts commit d973bb25585d13204d1a6e34f828504ac9359d71.

Signed-off-by: alphasaurs <alphasaurs@gmail.com>
Signed-off-by: alphasaurs <alphasaurs@gmail.com>
@@ -620,6 +620,7 @@ $ curl http://localhost:9090/api/v1/rules
],
"file": "/rules.yaml",
"interval": 60,
"limit": 0,
Copy link
Member

Choose a reason for hiding this comment

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

nit: no extra spacing is needed here.

Suggested change
"limit": 0,
"limit": 0,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@LeviHarrison
Copy link
Member

LeviHarrison commented Jan 11, 2022

let me know for tasks that needed to be completed before this release would like to learn and contribute more to this project

As the release will be cut tomorrow, I don't think there's anything left to get it, but you can certainly contribute to future releases. Nothing specific comes to mind right now, but you can always find more tasks in the issue tracker. I'm so glad for your enthusiasm in the project!

Signed-off-by: alphasaurs <alphasaurs@gmail.com>
Copy link
Member

@LeviHarrison LeviHarrison left a comment

Choose a reason for hiding this comment

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

Great! Thank you so much, this has been needing to get done for a while.

@LeviHarrison LeviHarrison merged commit 2f4289a into prometheus:main Jan 12, 2022
@LeviHarrison LeviHarrison mentioned this pull request Jan 12, 2022
suyashtava pushed a commit to suyashtava/prometheus that referenced this pull request Jan 18, 2022
Signed-off-by: suyashtava <suyashtava@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants