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

homebrew: Add support for services functions #8329

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

kitizz
Copy link
Contributor

@kitizz kitizz commented May 7, 2024

SUMMARY

Add a homebrew.services module for starting and stopping services that are attached to homebrew packages.

Fixes #8286.

ISSUE TYPE
  • Feature Pull Request
  • New Module/Plugin Pull Request
COMPONENT NAME

homebrew, homebrew_services

ADDITIONAL INFORMATION

Can now make sure a homebrew service is running with:

- name: Ensure the colima service is running
  community.general.homebrew_services:
    name: colima
    state: present

@ansibullbot

This comment was marked as outdated.

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added WIP Work in progress feature This issue/PR relates to a feature request integration tests/integration module module module_utils module_utils plugins plugin (any type) tests tests unit tests/unit labels May 7, 2024
@felixfontein felixfontein added the check-before-release PR will be looked at again shortly before release and merged if possible. label May 7, 2024
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Here are some first comments.

plugins/modules/homebrew_services.py Outdated Show resolved Hide resolved
plugins/modules/homebrew_services.py Outdated Show resolved Hide resolved
plugins/modules/homebrew_services.py Outdated Show resolved Hide resolved
plugins/modules/homebrew_services.py Outdated Show resolved Hide resolved
tests/integration/targets/black/handlers/main.yml Outdated Show resolved Hide resolved
@ansibullbot

This comment was marked as outdated.

@felixfontein felixfontein added the backport-9 Automatically create a backport for the stable-9 branch label May 20, 2024
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label May 28, 2024
@ansibullbot ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Jun 2, 2024
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Jun 3, 2024
@kitizz
Copy link
Contributor Author

kitizz commented Jun 5, 2024

Hi @felixfontein, who should I put as the author for this? I'm happy to be a maintainer if that's expected of me. Otherwise if existing authors for the homebrew.py package makes more sense, happy to do that do.

@kitizz kitizz changed the title [WIP] homebrew: Add support for services functions homebrew: Add support for services functions Jun 7, 2024
@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed WIP Work in progress labels Jun 7, 2024
@felixfontein
Copy link
Collaborator

Hi @felixfontein, who should I put as the author for this? I'm happy to be a maintainer if that's expected of me. Otherwise if existing authors for the homebrew.py package makes more sense, happy to do that do.

Since you created it, I would put yourself in there :) Depending on how much you copied over from the existing module, adding the author(s) from the original module also makes sense. (If you used code from existing modules, it's best to copy over their copyright lines so it's clear that you didn't create everything yourself. Adding the authors of the other module as authors is optional, but the copyright lines are not.)

If you add a new module or plugin we expect you to at least maintain it for some time, so you'd also need to add yourself to BOTMETA (see https://github.com/ansible-collections/community.general/blob/main/CONTRIBUTING.md#creating-new-modules-or-plugins for details).

name:
description:
- An installed homebrew package whose service is to be updated.
aliases: [ 'formula', 'package', 'pkg' ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general adding aliases should be avoided in not necessary. I can see why formula makes sense, but are package and pkg really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly matching the aliases in the other homebrew modules for consistency. But if these are unorthodox, I'm happy to nix them.

plugins/modules/homebrew_services.py Outdated Show resolved Hide resolved
@kitizz
Copy link
Contributor Author

kitizz commented Jun 9, 2024

Since you created it, I would put yourself in there :) ...

Yup! Makes sense. Only too happy to.

Fixes ansible-collections#8286.
Add a homebrew.services module for starting and stopping services
that are attached to homebrew packages.
@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI and removed ci_verified Push fixes to PR branch to re-run CI labels Jun 9, 2024
@kitizz
Copy link
Contributor Author

kitizz commented Jun 9, 2024

Hmmm, not sure what to make of the following error:

07:34 fatal: [testhost]: FAILED! => {
07:34     "changed": false,
07:34     "module_stderr": "shell-init: error retrieving current directory: getcwd: cannot access parent directories: Permission denied\njob-working-directory: error retrieving current directory: getcwd: cannot access parent directories: Permission denied\nTraceback (most recent call last):\n  File \"<stdin>\", line 133, in <module>\n  File \"<stdin>\", line 125, in _ansiballz_main\n  File \"<stdin>\", line 73, in invoke_module\n  File \"<frozen runpy>\", line 226, in run_module\n  File \"<frozen runpy>\", line 98, in _run_module_code\n  File \"<frozen runpy>\", line 88, in _run_code\n  File \"/tmp/ansible_community.general.homebrew_services_payload_eupguqp0/ansible_community.general.homebrew_services_payload.zip/ansible_collections/community/general/plugins/modules/homebrew_services.py\", line 279, in <module>\n  File \"/tmp/ansible_community.general.homebrew_services_payload_eupguqp0/ansible_community.general.homebrew_services_payload.zip/ansible_collections/community/general/plugins/modules/homebrew_services.py\", line 264, in main\n  File \"/tmp/ansible_community.general.homebrew_services_payload_eupguqp0/ansible_community.general.homebrew_services_payload.zip/ansible_collections/community/general/plugins/modules/homebrew_services.py\", line 183, in start_service\n  File \"/tmp/ansible_community.general.homebrew_services_payload_eupguqp0/ansible_community.general.homebrew_services_payload.zip/ansible_collections/community/general/plugins/modules/homebrew_services.py\", line 150, in _brew_service_state\n  File \"/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/json/__init__.py\", line 346, in loads\n    return _default_decoder.decode(s)\n           ^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File \"/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/json/decoder.py\", line 337, in decode\n    obj, end = self.raw_decode(s, idx=_w(s, 0).end())\n               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File \"/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/json/decoder.py\", line 355, in raw_decode\n    raise JSONDecodeError(\"Expecting value\", s, err.value) from None\njson.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)\n",
07:34     "module_stdout": "",
07:34     "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error",
07:34     "rc": 1
07:34 }

There seem to be multiple errors. One about getcwd permissions and one error parsing the JSON output of the services call. But I can't reproduce those on my system. Is there some extra stuff I need to set up for permissions in the Mac tests for CI?

@ansibullbot ansibullbot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Jun 9, 2024
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

The complete stderr in readable form is:

shell-init: error retrieving current directory: getcwd: cannot access parent directories: Permission denied
job-working-directory: error retrieving current directory: getcwd: cannot access parent directories: Permission denied
Traceback (most recent call last):
  File "<stdin>", line 133, in <module>
  File "<stdin>", line 125, in _ansiballz_main
  File "<stdin>", line 73, in invoke_module
  File "<frozen runpy>", line 226, in run_module
  File "<frozen runpy>", line 98, in _run_module_code
  File "<frozen runpy>", line 88, in _run_code
  File "/tmp/ansible_community.general.homebrew_services_payload_eupguqp0/ansible_community.general.homebrew_services_payload.zip/ansible_collections/community/general/plugins/modules/homebrew_services.py", line 279, in <module>
  File "/tmp/ansible_community.general.homebrew_services_payload_eupguqp0/ansible_community.general.homebrew_services_payload.zip/ansible_collections/community/general/plugins/modules/homebrew_services.py", line 264, in main
  File "/tmp/ansible_community.general.homebrew_services_payload_eupguqp0/ansible_community.general.homebrew_services_payload.zip/ansible_collections/community/general/plugins/modules/homebrew_services.py", line 183, in start_service
  File "/tmp/ansible_community.general.homebrew_services_payload_eupguqp0/ansible_community.general.homebrew_services_payload.zip/ansible_collections/community/general/plugins/modules/homebrew_services.py", line 150, in _brew_service_state
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

My guess is that

shell-init: error retrieving current directory: getcwd: cannot access parent directories: Permission denied
job-working-directory: error retrieving current directory: getcwd: cannot access parent directories: Permission denied

are warnings printed by brew services info black --json, and it didn't output JSON on stdout.

I would probably add a command task to the tests before Start the black service that simply runs brew services info black --json. Then maybe it's easier to see what's going wrong? I've added it below as a suggestion and will commit it.

@felixfontein
Copy link
Collaborator

While it doesn't show where the messages on stderr come from, it shows why stdout isn't JSON: it starts with two non-JSON lines

Hide this warning by setting HOMEBREW_SERVICES_NO_DOMAIN_WARNING.
Hide these hints with HOMEBREW_NO_ENV_HINTS (see `man brew`).

(And seriously, dumping stuff on stdout when the user is explicitly asking for JSON is a really bad idea.)

It also shows that you should use better error handling and not simply assume that stdout is JSON :)

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-9 Automatically create a backport for the stable-9 branch check-before-release PR will be looked at again shortly before release and merged if possible. feature This issue/PR relates to a feature request integration tests/integration module_utils module_utils module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

homebrew: Add support for services functions
3 participants