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

Allow to use constraints files for new-ansible and prepare #546

Merged
merged 6 commits into from
Sep 27, 2023

Conversation

felixfontein
Copy link
Collaborator

@felixfontein felixfontein commented Sep 26, 2023

@felixfontein felixfontein marked this pull request as draft September 26, 2023 19:40
@felixfontein
Copy link
Collaborator Author

@gotmax23 since we want to release 9.0.0a1 tomorrow (or if not then, soon), and since we have four collections whose latest versions aren't tagged (ansible-community/ansible-build-data#223 (comment)), we really need some mechanism to enforce constraints / override versions.

I've started adding minimal changes to antsibull-core (see ansible-community/antsibull-core#99) to allow restricting collection versions, created a PR for ansible-build-data with the constraints needed (ansible-community/ansible-build-data#300), and did the main impementation here to allow constraints files.

What do you think?

@felixfontein
Copy link
Collaborator Author

(CI will fail until the other two PRs have been merged.)

This makes it easier to adjust that code without having to modify ansible-core
and have a new release of it, and depend on that new release.
@felixfontein
Copy link
Collaborator Author

I decided to not use GalaxyClient.get_latest_matching_version() any longer. That means we don't need a change to antsibull-core to implement this, which makes it easier to get this released ASAP.

I think this logic is better placed here anyway since antsibull's purpose (among others) is to determine the versions of collections to be included in Ansible, so offloading parts of that logic to another library (which only wants to provide a rather minimalistic interface for that) doesn't feel right anyway.

Copy link
Contributor

@gotmax23 gotmax23 left a comment

Choose a reason for hiding this comment

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

I gave this a cursory look. Thanks for working on it!

Comment on lines +307 to +313
build_step_parser.add_argument(
"--constraints-file",
default=None,
help="File containing a list of constraints for collections"
" included in Ansible. This is considered to be relative to"
" --build-data-dir. The default is"
" $BASENAME_OF_BUILD_FILE-X.Y.constraints",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create a constraints_parser that contains this argument that build_step_parserandnew_parser` inherit instead of defining this argument twice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The descriptions for the two options differ w.r.t. the default value. One uses the basename of the build file, while the other uses the basename of the pieces file. That's also why --build-file both shows up in the same two parsers.

src/antsibull/build_ansible_commands.py Show resolved Hide resolved
src/antsibull/cli/antsibull_build.py Show resolved Hide resolved
src/antsibull/build_ansible_commands.py Outdated Show resolved Hide resolved
src/antsibull/versions.py Show resolved Hide resolved
@gotmax23
Copy link
Contributor

Triggering CI

@gotmax23 gotmax23 closed this Sep 27, 2023
@gotmax23 gotmax23 reopened this Sep 27, 2023
Co-authored-by: Maxwell G <maxwell@gtmx.me>
@gotmax23
Copy link
Contributor

Other than that, I think we can merge and release this.

Co-authored-by: Maxwell G <maxwell@gtmx.me>
@felixfontein felixfontein marked this pull request as ready for review September 27, 2023 16:35
@felixfontein
Copy link
Collaborator Author

Pyre complains: src/antsibull/versions.py:129:44 Incompatible parameter type [6]: In call `os.fspath`, for 1st positional argument, expected `str` but got `Union[PathLike[str], str]`.

This reverts commit 94b39a5.
@felixfontein
Copy link
Collaborator Author

I reverted this so we can get this merged and released. Let's re-do it in a follow-up afterwards.

@felixfontein felixfontein merged commit 2c0987b into ansible-community:main Sep 27, 2023
8 checks passed
@felixfontein felixfontein deleted the constraints branch September 27, 2023 17:01
@felixfontein
Copy link
Collaborator Author

@gotmax23 thanks a lot for reviewing this!

@felixfontein
Copy link
Collaborator Author

#548 re-does the filename type changes.

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.

2 participants