Make rotate_artifacts accessible from env/settings#1417
Make rotate_artifacts accessible from env/settings#1417fkuep wants to merge 1 commit intoansible:develfrom
Conversation
Related to ansible#1415 Make `rotate_artifacts` accessible from the settings file. * Modify `src/ansible_runner/config/_base.py`: - Retrieve `rotate_artifacts` from `settings` dictionary in `prepare_env` method. - Set `rotate_artifacts` in `__init__` method only if not provided in `settings` dictionary. - Ensure `rotate_artifacts` is set to 0 if not provided in command line or settings file. - Make `rotate_artifacts` default to 0 in `__init__` method. - Set `rotate_artifacts` from command line if provided. * Modify `demo/env/settings`: - Add `rotate_artifacts` option to the settings file. --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/ansible/ansible-runner/issues/1415?shareId=XXXX-XXXX-XXXX-XXXX).
|
This one seems to work, only I wonder if the settings file should really win over the command line argument. |
Shrews
left a comment
Shrews
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I think you're on the right track. I think all you really need is your change on line 230 of _base.py. The other changes seem unnecessary, unless there is something I'm not seeing?
As for the settings file contents winning over the command line, I probably would have reversed that if I had had a say at the time the project was created, but it is what it is, so let's just be consistent with the other settings values.
You'll also need to update the documentation in intro.rst for the env/settings section.
Also, you'll want to add some tests to validate this works as expected (we won't accept it without tests). You might need to be a bit creative here. When my group took over this code base, we worked to improve the existing set of tests, but we are lacking in certain areas still, and I'm not sure if we currently have a good example for you to use for a new test. I took a very quick look through the existing tests and didn't see any good candidates. See what you can come up with. If you have problems, I can try to assist you, but may be delayed in getting around to it.
| container_auth_data=None, | ||
| ident: str | None = None, | ||
| rotate_artifacts: int = 0, | ||
| rotate_artifacts: int | None = None, |
There was a problem hiding this comment.
I'm not seeing a reason for this change. If you revert it back, you can also just revert your change below on line 113, and it will work as you expect.
2 participants
Related to #1415
Make
rotate_artifactsaccessible from the settings file.Modify
src/ansible_runner/config/_base.py:rotate_artifactsfromsettingsdictionary inprepare_envmethod.rotate_artifactsin__init__method only if not provided insettingsdictionary.rotate_artifactsis set to 0 if not provided in command line or settings file.rotate_artifactsdefault to 0 in__init__method.rotate_artifactsfrom command line if provided.Modify
demo/env/settings:rotate_artifactsoption to the settings file.For more details, open the Copilot Workspace session.