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

Implement Resource limits and reserves Modules #104

Merged
merged 6 commits into from
May 8, 2021

Conversation

apavanello
Copy link
Contributor

PR ref #103

Add two new lib files (library/dokku_resource_limit.py and library/dokku_resource_reserve.py) and update Readme

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks a lot for the contribution @apavanello, very welcome!

Please see below a few comments - I noticed too late that I made my suggestions in the README, i.e. you'll need to move them to the source file (sorry).

It would be nice to add a test of this new functionality somewhere here but for this we first need an app installed during the verify stage. Unfortunately, my PR #98 that would do this is blocked by a longstanding molecule bug and there doesn't seem to be any action on it... until that point no need to add a test

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
library/dokku_resource_reserve.py Outdated Show resolved Hide resolved
alpsilva added 2 commits May 6, 2021 22:27
Change Allowed_keys to be agnostic
@apavanello
Copy link
Contributor Author

Hi,

Maked changes on source to create a correct README and maked a allowed_keys to be agnostic, in fact i don't need check the output of dokku --quiet resource:limit myapp because the output will have only two returns, the full list of resources (even when no have limits/reserves) and the error (when the app doesn't exist), so i use the current output to generate the allowed_keys list, when dokku add new resources no need to change the list

In test subject, i created a branch in my repo and added some tests and all have pass (you can check in my github actions https://github.com/apavanello/ansible-dokku/actions/runs/818794950), when #98 be fixed i can make another PR to implement the resources tests

Sorry for english mistakes,i still learning the language

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

in fact i don't need check the output of dokku --quiet resource:limit myapp because the output will have only two returns, the full list of resources (even when no have limits/reserves) and the error (when the app doesn't exist), so i use the current output to generate the allowed_keys list, when dokku add new resources no need to change the list

great!

In test subject, i created a branch in my repo and added some tests and all have pass (you can check in my github actions https://github.com/apavanello/ansible-dokku/actions/runs/818794950)

Great, thanks for this!

when #98 be fixed i can make another PR to implement the resources tests

Cheers, I think I started to get an idea of where the molecule issue comes from. Will ping you once this is done (but we will merge this PR nevertheless).

Sorry for english mistakes,i still learning the language

No worries!

The changes look good, just some final requests.

library/dokku_resource_reserve.py Outdated Show resolved Hide resolved
library/dokku_resource_reserve.py Outdated Show resolved Hide resolved
library/dokku_resource_reserve.py Show resolved Hide resolved
library/dokku_resource_limit.py Outdated Show resolved Hide resolved
library/dokku_resource_limit.py Outdated Show resolved Hide resolved
library/dokku_resource_limit.py Outdated Show resolved Hide resolved
library/dokku_resource_limit.py Outdated Show resolved Hide resolved
library/dokku_resource_limit.py Outdated Show resolved Hide resolved
library/dokku_resource_reserve.py Outdated Show resolved Hide resolved
library/dokku_resource_reserve.py Outdated Show resolved Hide resolved
SMALL FIXES IN README
@apavanello
Copy link
Contributor Author

@ltalirz all done, changes sugested are maded.

library/dokku_resource_limit.py Outdated Show resolved Hide resolved
library/dokku_resource_reserve.py Outdated Show resolved Hide resolved
@ltalirz ltalirz self-requested a review May 8, 2021 08:23
@ltalirz ltalirz merged commit 4427ea8 into dokku:master May 8, 2021
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.

2 participants