search

dokku / ansible-dokku

Ansible modules for installing and configuring Dokku
MIT License
156 stars 44 forks source link

Fix dokku_acl_app and dokku_acl_service ansible libraries #150

Closed dvessey-fes closed 1 year ago

dvessey-fes commented 1 year ago

This PR provides a couple of minor fixes:

  1. the dokku commands that list ACL information return the data on stderr, however the plugin was trying to read it from stdout. Added a redirect_stderr option on subprocess_check_output (which defaults to False, to avoid breaking usage anywhere else), and then modified dokku_acl_app and dokku_acl_service to set that to True
  2. The dokku_acl_service module wasn't using the right dokku acl:... commands to list/add/remove

This change is pretty small, I've tested this locally to confirm it works. Let me know if anything else is needed!

dvessey-fes commented 1 year ago

Just checking in on this one, to see if there's anything extra needed from me? those CI task failures don't seem to be related to these changes.

ltalirz commented 1 year ago

Thank you for this contribution @dvessey-fes (somehow I overlooked the notification).

Let me try to figure out what's going on in our CI setup. In the meanwhile, do you know when the API for the dokku_acl_service changed - in other words do your changes require a minimal dokku version?

dvessey-fes commented 1 year ago

@ltalirz not sure about when the behaviour changed - I've only been using dokku for a few months now. but at least since 0.26.7

ltalirz commented 1 year ago

Thanks @dvessey-fes , sorry for the long wait.

Would you be able to rebase on the current master?

Tests will still fail at the verify stage https://github.com/dokku/ansible-dokku/issues/154, but at least we'll know that the role runs through fine.

dvessey-fes commented 1 year ago

rebased & pushed branch!

ltalirz commented 1 year ago

Somehow this rebase includes my commits on top of master; also there are some minor formatter complaints.

I'll fix this in https://github.com/dokku/ansible-dokku/pull/155, thanks for the contribution!