Typical Time for Review of a New Module Pull Request?

Archives Ansible Developer
1

Hi,

I submitted a PR for a new module to manage AWS ses identity: https://github.com/ansible/ansible/pull/31140

This has been open without any comments for 25 days. I'm a first time contributor so was really hoping to get some feedback on whether this is a good contribution so I can adjust it if necessary.

I know this is a very busy project, so just hoping to get an idea of how long it typically is before new module PRs like this get looked at.

Thanks
Ed Costello

2

I think it varies based on whether others are also in need of the same functionality.

If you can find a friend to test and comment on the PR that is usually a good help.

I see you have created tests, that’s a big help too.

You can always ask core team to review as part of a public core meeting - details of the meetings are here: https://github.com/ansible/community/blob/master/meetings/README.md

Hope this helps

Jon

3

Hi Ed,

I’m also a first time contributor to Ansible. I’ve got one minor change as a pull request and when that goes through I’m waiting to submit (https://github.com/ansible/ansible/pull/32065) and then I’ve got a new module to submit.

Perhaps we could review and comment on each others code? I’ll certainly make an effort this week to try out your module and leave comments on github. Is it correct for a newbie to review changes and approve or should I only leave comment without explicit approval?

Regards

Andy

4

Thanks Andy,

If I’m reading the bot help[1] right I don’t think we’re able to approve each other’s change requests. My understanding is that a PR always needs approval from a module maintainer (or maintainer of module in the same namespace), which makes sense really. Of course more eyes are always good on these things. If you do get a chance to have a look I’d appreciate any feedback.

I’ve had a look over your pull request, it looks like a good change to me so I’ve +1ed as suggested in the doc as I’m in favour of these kind of small fix, quick win changes getting done.

The only thing I did note which may or may not help you in the short term is that it looks like your bugfix is unecesary if you’re able to run python 3.6 rather than 3.5. It looks like in 3.6 the json.loads method was changed[2] to accept bytes as well. Looks like ansible’s targeting python 3.5 so your fix is still valid. I just mention this incase it helps while waiting.

I’m going to give it a bit longer to let the people I work with throw rocks at the SES module (they’ve already found one bug that I’ve pached) and then will probably try and raise it at a public core meeting as Jon suggested.

1: https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md#when-will-your-pull-request-be-merged
2: https://docs.python.org/3.6/library/json.html#json.loads

Cheers

Ed

5

Thanks Ed,
Thats all mega helpful. I’ve added the note about Python 3.5 to 3.6 change to the module comments.
I tested your aws ses module too and +1ed. Couldn’t see any issues with my test. It will be a helpful addtion!
Best Regards
Andy