10000 add quota support to system backend by tot19 · Pull Request #1092 · hvac/hvac · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

add quota support to system backend #1092

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
Merged
merged 3 commits into from
Dec 30, 2023
Merged

add quota support to system backend #1092

merged 3 commits into from
Dec 30, 2023

Conversation

tot19
Copy link
Contributor
@tot19 tot19 commented Oct 16, 2023 •
edited by briantist
Loading

This is my first commit to this repo, so please let me know if there are any edits I need to make! Thanks again for all of your awesome work on this project.

Resolves #1071 - added system backend support and integration tests for quotas

8000 @tot19 tot19 requested a review from a team as a code owner October 16, 2023 09:11
briantist
briantist reviewed Oct 16, 2023
View reviewed changes
Copy link
Contributor
@briantist briantist left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Welcome @tot19 , thanks very much for this PR. It'll take me some time to properly review but looks good so far aside from the small things I mentioned. It might also be good to add a new documentation page for this (see the other docs pages) with examples and explanation.

Since v2.0.0 is imminent, this will be a post-v2 addition.

@briantist briantist added enhancement a new feature or addition system backend generally related to the Vault system backend routes minor Used as part of release-drafter's version-resolver configuration labels Oct 16, 2023
@tot19
Copy link
Contributor Author
tot19 commented Oct 16, 2023

Made the suggested code changes. Thank you for pointing those out. Will submit another PR shortly with documentation add

@briantist briantist changed the title Issue #1071 - added system backend support and integration tests for quotas add quota support to system backend Oct 16, 2023
@briantist
Copy link
Contributor

Made the suggested code changes. Thank you for pointing those out. Will submit another PR shortly with documentation add

Please include documentation for this feature in this PR, not in a new one, that way it can all be reviewed and merged together, thanks!

@tot19
Copy link
Contributor Author
tot19 commented Oct 16, 2023

You got it! Will get that fixed up within next 24 hours. Greatly appreciate the feedback

@briantist
Copy link
Contributor

There are some lint failures as well. Be sure to run poetry run black ., poetry run flake8 ., and make typos locally to catch those. I need to approve CI for each commit pushed for new contributors so you'll get faster feedback if you run linting and tests locally.

You got it! Will get that fixed up within next 24 hours. Greatly appreciate the feedback

No rush! I'll be traveling for the next week, my time to review will be limited.

@tot19
Copy link
Contributor Author
tot19 commented Oct 17, 2023

Thank you! still new to using a lot of these tools, so your patience/help is greatly appreciated.

@tot19 tot19 force-pushed the issue1071 branch 3 times, most recently from 91fe3f1 to 567685a Compare October 19, 2023 07:22
@tot19
Copy link
Contributor Author
tot19 commented Oct 19, 2023

Ah I didn't know I could enable github actions on my fork! That is so cool. Confirmed that all of the tests passed in my fork this time around. Once again, thank you for the patience.

@briantist briantist added this to the v2.1.0 milestone Oct 21, 2023
@briantist briantist self-assigned this Oct 21, 2023
@codecov
Copy link
codecov bot commented Oct 21, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.19%. Comparing base (6fcd885) to head (adbd828).
Report is 37 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1092      +/-   ##
==========================================
+ Coverage   87.12%   87.19%   +0.06%     
==========================================
  Files          64       65       +1     
  Lines        3162     3179      +17     
==========================================
+ Hits         2755     2772      +17     
  Misses        407      407
Files Coverage Δ
hvac/api/system_backend/__init__.py 100.00% <100.00%> (ø)
hvac/api/system_backend/quota.py 100.00% <100.00%> (ø)

@briantist briantist added the hacktoberfest-accepted PR is accepted for hacktoberfest label Oct 21, 2023
@tot19 tot19 force-pushed the issue1071 branch 2 times, most recently from 8ecce96 to fa32787 Compare October 30, 2023 02:25
briantist
briantist reviewed Oct 30, 2023
View reviewed changes
@tot19
Copy link
Contributor Author
tot19 commented Oct 30, 2023

1.15.0 added new parameter 'inheritable.' Fixing tests now

@tot19
Copy link
Contributor Author
tot19 commented Oct 30, 2023

So I spent a couple of hours fixing 1.15.0 tests with my local 1.15.0 enterprise binary to discover that 1.15.0 non-enterprise bounces all requests to quotas endpoint with a 400 request. Ultimately added a skipIf for version >= 1.15.0 due to this change

@briantist
Copy link
Contributor

So I spent a couple of hours fixing 1.15.0 tests with my local 1.15.0 enterprise binary to discover that 1.15.0 non-enterprise bounces all requests to quotas endpoint with a 400 request. Ultimately added a skipIf for version >= 1.15.0 due to this change

Thanks, I might try to go and put modern enterprise versions in our CI soon, so maybe I can get that done before we merge this..

@briantist
Copy link
Contributor
briantist commented Nov 23, 2023 •
edited
Loading

So I spent a couple of hours fixing 1.15.0 tests with my local 1.15.0 enterprise binary to discover that 1.15.0 non-enterprise bounces all requests to quotas endpoint with a 400 request. Ultimately added a skipIf for version >= 1.15.0 due to this change

Thanks, I might try to go and put modern enterprise versions in our CI soon, so maybe I can get that done before we merge this..

btw I haven't forgotten about this. Starting to think through how to do it led me to some other difficulties with our tests which ultimately led to:

briantist
briantist approved these changes Dec 30, 2023
View reviewed changes
Copy link
Contributor
@briantist briantist left a comment

Choose a reason for hiding this comment

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

@tot19 thank you, I don't think I'll be able to solve the vault enterprise issues soon enough and I don't want to hold this PR up any longer. Thanks for your patience!

@briantist briantist merged commit 0867ae6 into hvac:main Dec 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briantist briantist briantist approved these changes

Assignees

@briantist briantist

Labels
enhancement a new feature or addition hacktoberfest-accepted PR is accepted for hacktoberfest minor Used as part of release-drafter's version-resolver configuration system backend generally related to the Vault system backend routes
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.

Add Resource Quotas
2 participants
@tot19 @briantist
0