Heroku collaborator module #39001
Merged
Heroku collaborator module #39001
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The test The test The test |
ansibot
added
cloud
module
|
The test |
marns93
force-pushed
the
heroku-collaborator-module
branch
from
0e39558
to
12d1161
Compare
ansibot
removed
the
ci_verified
|
ready_for_review |
ansibot
added
core_review
Akasurde
requested changes
Apr 19, 2018
| @@ -0,0 +1,142 @@ | |||
| #!/usr/bin/python | |||
|
|
|||
| # Copyright: Ansible Project | |||
There was a problem hiding this comment.
Copyright: (c) 2018, Ansible Project
| required: true | ||
| state: | ||
| description: | ||
| - Create or remove the heroku collaborator |
There was a problem hiding this comment.
Add description like with all possible combinations -
If set to C(present) and heroku user is already collaborator, then ...
There was a problem hiding this comment.
Thank you, we added requested documentation.
| default: "present" | ||
| notes: | ||
| - C(HEROKU_API_KEY) and C(TF_VAR_HEROKU_API_KEY) env variable can be used instead setting c(api_key). | ||
| - If you use I(--check), you can also pass the I(-v) flag to see affected apps in msg. |
There was a problem hiding this comment.
New person will be confused when you refer to msg, adding a brief description will help.
There was a problem hiding this comment.
Here, we tried to add more documentation, but after several tries couldn't get YAML to validate. If you think that our current suggestion is still not acceptable, we would appreciate your help.
| - { user: 'x.y@example.com', apps: ["heroku-example-app"] } | ||
| ''' | ||
|
|
||
| RETURN = ''' # ''' |
There was a problem hiding this comment.
Return is not compulsory but will help end user to understand return value of module.
There was a problem hiding this comment.
The documentation says:
If your module doesn't return anything (apart from the standard returns), you can use ``RETURN = ''' # '''``.
Our module indeed doesn't return anything apart from the standard returns (msg). If you think that we should document something we would appreciate further hints.
|
|
||
| import os | ||
|
|
||
| DEPENDENCY_CHECK = True |
There was a problem hiding this comment.
Might want to change to HAS_HEROKU to match other Ansible modules code.
| def main(): | ||
| module = AnsibleModule( | ||
| argument_spec=dict( | ||
| api_key=dict(default=os.getenv('HEROKU_API_KEY', os.getenv('TF_VAR_HEROKU_API_KEY')), type='str'), |
There was a problem hiding this comment.
Use env_fallback
from ansible.module_utils.basic import env_fallback
...
api_key=dict(fallback=(env_fallback, ['HEROKU_API_KEY', TF_VAR_HEROKU_API_KEY']), type='str', no_log=True),
| module.fail_json(msg='Heroku authentication failure, please check your API Key') | ||
|
|
||
| # Prevent API key from appearing in debug logs | ||
| del module.params['api_key'] |
There was a problem hiding this comment.
no_log=True is argument spec will handle this.
| supports_check_mode=True | ||
| ) | ||
|
|
||
| if not DEPENDENCY_CHECK: |
There was a problem hiding this comment.
One suggestion, it will be helpful to create a common library under module_utils so that in future all Heroku module will use same like DigitalOcean or influxdb.
Akasurde
removed
the
needs_triage
ansibot
added
needs_revision
ansibot
added
stale_ci
|
@Akasurde thank you very much for your review and hints, we have implemented everything to the best of our knowledge. ready_for_review |
ansibot
added
core_review
Akasurde
approved these changes
Apr 27, 2018
|
LGTM. We need one more person to take a look and then we are good to go. |
ansibot
added
the
stale_ci
|
Hi @Akasurde, do you think you could help us to find a second reviewer? Quite honestly we're not quite sure as to how to proceed. Many thanks! |
|
@resmo @ryansb @willthames Could you please provide your valuable reviews ? |
resmo
suggested changes
May 19, 2018
lib/ansible/module_utils/heroku.py
Outdated
| @@ -0,0 +1,38 @@ | |||
| # Copyright: (c) 2018, Ansible Project | |||
| # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) | |||
There was a problem hiding this comment.
blocker: code under module_utils/* has to be BSD licensed.
| - If set to C(present) and heroku user is not collaborator, then add user to app. | ||
| - If set to C(absent) and heroku user is collaborator, then delete user from app. | ||
| author: | ||
| - Marcel Arns <marcel.arns@moneymeets.com> |
There was a problem hiding this comment.
not a blocker, but please replace your mail with your github handle.
| - Suppress email invitation when creating collaborator | ||
| type: bool | ||
| required: false | ||
| default: "False" |
There was a problem hiding this comment.
not a blocker, but it looks better in the docs if you use (yes/no) for type bool.
| api_key: | ||
| description: | ||
| - Heroku API key | ||
| required: false |
There was a problem hiding this comment.
not a blocker, but requried=false is the default and can be omitted.
ansibot
added
needs_revision
marns93
force-pushed
the
heroku-collaborator-module
branch
from
6f01c92
to
6c435d7
Compare
ansibot
removed
the
needs_rebase
|
@resmo thank you for the review, we have addressed your comments in 6f01c92. We have also rebased the PR against latest devel and squashed fixup commits. We have noticed that changelogs are apparently automatically generated now, so we have removed the changelog commit. If we've got it wrong, we will appreciate further guidance on your part. Many thanks! |
resmo
approved these changes
May 22, 2018
|
shipit |
ansibot
added
core_review
achinthagunasekara
pushed a commit
to achinthagunasekara/ansible
that referenced
this pull request
May 23, 2018
jacum
pushed a commit
to jacum/ansible
that referenced
this pull request
Jun 26, 2018
ilicmilan
pushed a commit
to ilicmilan/ansible
that referenced
this pull request
Nov 7, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
This module allows managing Heroku app collaborators. Heroku doesn't offer an interface that provides overview across all organization apps, therefore managing collaborators without Ansible is difficult and error-prone. The module solves this problem.
ISSUE TYPE
COMPONENT NAME
heroku_collaborator