add argument to update version in galaxy.yml file

[KB-perByte]

usage - antsibull-changelog release --version 6.0.0 --update-galaxy-file

updates the version in the changelog and also in the galaxy.yml

[gotmax23]

I'm a bit conflicted as to whether antsibull-changelog is the right place/level in the stack to handle bumping the version. Also, pyyaml is lossy and does not preserve comments or formatting which is problematic for galaxy.yml. @felixfontein, wdyt?

[gotmax23]

@KB-perByte, thanks for the PR! I left some feedback.

[gotmax23]

Also, a changelog fragment is needed for this change.

[KB-perByte]

I'm a bit conflicted as to whether antsibull-changelog is the right place/level in the stack to handle bumping the version. Also, pyyaml is lossy and does not preserve comments or formatting which is problematic for galaxy.yml. @felixfontein, wdyt?

Thanks for the quick review, @gotmax23
I have similar concerns the comments and formatting are definitely not preserved and it also hinders the yaml start (---).
But this would be great to have for the collections to consume in CI.
Ref : the push workflow a separate task needs to be invoked to update the galaxy.yml file.
and, I'll be adding tests, still figuring out better alternatives to update the yaml file.
Regards

[felixfontein]

Thanks for your contribution. I started adding comments earlier today, but then got interrupted... and now most of the things already changed ;) So here's a new set of comments.

[felixfontein]

I'm a bit conflicted as to whether antsibull-changelog is the right place/level in the stack to handle bumping the version. Also, pyyaml is lossy and does not preserve comments or formatting which is problematic for galaxy.yml. @felixfontein, wdyt?

I agree, this feels a bit wrong. For such changes ruamel is the better library, but I don't think we should add a dependency on a second YAML library just for that. Maybe it would make sense to add this to collection_prep, as it already has a similar tool (https://github.com/ansible-network/collection_prep/blob/master/collection_prep/cmd/version.py)?

[gotmax23]

I'm a bit conflicted as to whether antsibull-changelog is the right place/level in the stack to handle bumping the version. Also, pyyaml is lossy and does not preserve comments or formatting which is problematic for galaxy.yml. @felixfontein, wdyt?

I agree, this feels a bit wrong. For such changes ruamel is the better library, but I don't think we should add a dependency on a second YAML library just for that. Maybe it would make sense to add this to collection_prep, as it already has a similar tool (https://github.com/ansible-network/collection_prep/blob/master/collection_prep/cmd/version.py)?

I'm happy to merge this if you think there's a significant usecase, but I think the sed approach that preserves formatting is more ideal. Even ruamel.yaml doesn't preserve formatting, just comments, and I agree that it doesn't make sense to add that as an extra dependency. I'm not sure about the current maintenance status of collection_prep, but yeah, that might be a better place.

[KB-perByte]

@gotmax23 @felixfontein Thanks for the quick and detailed reviews,
I am not sure about collection_prep being the right place for this change, as the changelog version is added and decided with the antsibull-changelog command itself. thinking deep
Regards

[gotmax23]

@KB-perByte, do you plan to fix the lint errors and address the remaining feedback?

[KB-perByte]

@gotmax23 this completely went out of my radar, l'll update the PR.
Thank you.

[KB-perByte]

I am trying to add unit tests! WIP

[gotmax23]

Thanks for updating this! I have some follow ups :).

[gotmax23]

If galaxy_path is None but --update-galaxy-file is passed, this should raise an error instead of continuing silently.

[gotmax23]

I think it should return 1 and exit with an error code, not just print a log message.

[KB-perByte]

making galaxy_path : str from galaxy_path : StrOrBytesPath for the following errors

src/antsibull_changelog/update_galaxy.py:27: error: Argument 1 to "load_yaml" has incompatible type "str | bytes | PathLike[str] | PathLike[bytes]"; expected "str"  [arg-type]
src/antsibull_changelog/update_galaxy.py:33: error: Argument 1 to "store_yaml" has incompatible type "str | bytes | PathLike[str] | PathLike[bytes]"; expected "str"  [arg-type]
src/antsibull_changelog/cli.py:623: error: Argument "galaxy_path" to "update_galaxy" has incompatible type "str | None"; expected "str | bytes | PathLike[str] | PathLike[bytes]"  [arg-type]

[gotmax23]

making galaxy_path : str from galaxy_path : StrOrBytesPath for the following errors

Ah, right. We also need to update the load_yaml and store_yaml signatures... You can revert my suggestion in that case.

src/antsibull_changelog/cli.py:623: error: Argument "galaxy_path" to "update_galaxy" has incompatible type "str | None"; expected "str | bytes | PathLike[str] | PathLike[bytes]" [arg-type]

This one seems legitimate. The calling code should make sure that the function isn't called when that variable is None.

[gotmax23]

It looks like you added a fixture but didn't make any test changes?

[KB-perByte]

I haven't added any tests yet.

[KB-perByte]

@gotmax23 I have this specific use case to preserve the yaml start (---) wherever it is present, ruamel is a valid candidate as per my need. ref
Would you be fine with a yaml wrapper that uses ruamel?
I personally don't like the idea, but I am running out of substitutes.

[gotmax23]

Yeah, ruamel.yaml would be better here, but I'm not particularly inclined to add another dependency; we already use pyyaml through antsibull_core.yaml (edit: antsibull-changelog has its own copy of the code. It doesn't depend on antsibull_core). See #127 (comment). I'm happy to maintain a short bit of code for this, but I don't want this to turn into a big thing when it's already a bit outside the scope of the project.

[felixfontein]

This file isn't used so far.

[felixfontein]

@KB-perByte this PR hasn't progressed in a year. Are you still planning to work on this or can we close it? Thanks.

[KB-perByte]

We can close this off! Thanks

[felixfontein]

Thank you!