Skip to content

Conversation

@StevenH1901
Copy link
Contributor

Per the section_module docs (and all other modules), app_id is not a required parameter and should default to 'ansible'.

When running version 1.7.0, I receive an error saying that 'app_id' is a required parameter. This error may be present in previous versions, but I have not verified.

Fixes #123

@cmeissner cmeissner self-requested a review January 11, 2025 10:16
Copy link
Member

@cmeissner cmeissner left a comment

Choose a reason for hiding this comment

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

HI @StevenH1901,

Thank you for opening this PR, Some things need to be in place before we can merge it.

  1. combine all fix typo commits to a single commit
  2. add a test for checking the behavior of the app_id parameter, ideally for all common parameters
  3. extend the section test to check the list_order parameter.

@StevenH1901
Copy link
Contributor Author

@cmeissner - Not 100% sure if this is what you were expecting, but if not let me know and I can get it fixed.

I'm also not sure how to combine two commits that have different commits between them.

@cmeissner
Copy link
Member

@cmeissner - Not 100% sure if this is what you were expecting, but if not let me know and I can get it fixed.

I'm also not sure how to combine two commits that have different commits between them.

Thank you for adding/extending the requested tests. I did not find the time to run it on my system. May I ask you to enable the CI workflow in your account, to see the result of it. After that, I can allow PR of you to trigger workflow runs in the project.

I will try to review your PR completely on the weekend.

Fix typos in `address` and `section` module. Typo in `section` module
could have functional impact.
Update `app_id` to be not required and set default to ansible as per documentation.
Copy link
Member

@cmeissner cmeissner left a comment

Choose a reason for hiding this comment

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

LGTM

StevenH1901 and others added 4 commits April 18, 2025 13:27
* make tool chain work with podman too
* update db setup script to work with podman
* set predictable container names
* fix fissues with starting docker-compose in CI
  * upgrade `docker-compose` before run phpipam-action
  * pin python to version 3.9 as long we did not found time to test newer versions

run stale workflow on hosted runners
* upgrade `docker-compose` before run phpipam-action
* pin python to version 3.9 as long we did not found time to test newer versions
@cmeissner cmeissner merged commit cd10076 into codeaffen:develop Apr 18, 2025
5 checks passed
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.

app_id is a required parameter despite documentation saying otherwise

2 participants