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

Fixes #37958 - Add "Sync Dependencies" option to Ansible collection repositories #11195

Conversation

Thorben-D
Copy link
Contributor

Redmine issue

This PR adds a new option, "Sync dependencies", to Ansible collection repositories.

When enabled (default), Pulp will sync the collections given in the requirements file and their dependencies.

When disabled, Pulp will only sync the collections, without their dependencies.

On the backend side, this is done by passing sync_dependencies to pulp_ansible.
I decided to keep track of where this setting is enabled at the root repository level, should we ever want to extend this functionality past Ansible collections.

@sjha4
Copy link
Member

sjha4 commented Oct 30, 2024

Code looks good. Would you be able to provide an example live repo to test this with? We'll want to add some automation around testing this.

The failing tests on this are VCR tests. Atix folks have previously recorded VCRs but if you face any issues, let me know and I can help with that..

@sjha4 sjha4 self-assigned this Oct 30, 2024
@Thorben-D Thorben-D force-pushed the issue/37958_add_sync_deps_option_ansible_collection branch from e3d8c1a to 511e6d1 Compare November 6, 2024 14:24
@Thorben-D
Copy link
Contributor Author

The VCRs should be good now, but I am struggling a bit with the test.
Really all that needs to be checked is the inclusion of the sync_dependencies parameter in the request to the Pulp API.
Whether the parameter does what it says is tested in pulp_ansible.
Do you have a pointer for me as to where such a test should go?

@sjha4
Copy link
Member

sjha4 commented Nov 8, 2024

There are some helper methods you could use here in the sync test:

The data will need to be set up in fixture here:

pulp3_ansible_collection_root_1:

I imagine something like for the test:

pulp_remote = @repo.backend_service(@primary).get_remote
assert_equal pulp_remote.sync_dependencies, @repo.sync_dependencies

@Thorben-D Thorben-D force-pushed the issue/37958_add_sync_deps_option_ansible_collection branch from 511e6d1 to f6fd4f0 Compare November 15, 2024 11:02
@Thorben-D
Copy link
Contributor Author

Thanks for your help! Test is added.

- 860cc24bec6e4aa8ad51bb36e23a67de
=======
Copy link
Member

Choose a reason for hiding this comment

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

The VCRs seem to have merge conflicts of some sort..What you can do is restore these files, rebase and then re-record the sync test..

@Thorben-D Thorben-D force-pushed the issue/37958_add_sync_deps_option_ansible_collection branch 3 times, most recently from 34440d9 to 466e685 Compare November 19, 2024 11:52
@Thorben-D Thorben-D force-pushed the issue/37958_add_sync_deps_option_ansible_collection branch from 466e685 to 5537e6a Compare November 26, 2024 13:23
@sbernhard
Copy link
Member

@sjha4 ready for review again :-)

@Thorben-D Thorben-D force-pushed the issue/37958_add_sync_deps_option_ansible_collection branch from 5537e6a to 433876a Compare November 27, 2024 12:27
@bastian-src
Copy link
Contributor

Tested latest changes, LGTM!

@sbernhard sbernhard merged commit 58dfb8c into Katello:master Nov 27, 2024
26 of 27 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.

4 participants