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.

Sign up for GitHub

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

Jump to bottom

Add dependent collections to galaxy.yml and requirements.yml #2145

Closed
wants to merge 3 commits into from
Closed

Add dependent collections to galaxy.yml and requirements.yml #2145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2d5716d
Add dependent collections to requirements.yml
GomathiselviS Jun 25, 2024
c556609
Add dependencies to galaxy.yml
GomathiselviS Jun 25, 2024
175d041
Add changelog fragment
GomathiselviS Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelogs/fragments/add_dependencies_to_galaxy.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
trivial:
- Add dependencies to galaxy.yml(https://github.com/ansible-collections/amazon.aws/pull/2145).
8 changes: 8 additions & 0 deletions galaxy.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@ description: A variety of Ansible content to help automate the management of AWS
license_file: COPYING
tags: [amazon, aws, cloud]
repository: https://github.com/ansible-collections/amazon.aws
dependencies:
"ansible.windows": "*"
Copy link
Contributor

@alinabuzachis alinabuzachis Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These dependencies are only needed for integration tests. Is there a way to add them only in the file tests/integration/requirements.yml and use this file to pull the dependencies on zuul?

Copy link
Contributor

Choose a reason for hiding this comment

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

This should not appear here, the collection does not have any dependencies with these collections.
This is needed for test purposes only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding, collections specified in tests/integration/requirements.yml are not installed by Zuul. If we remove these collections from the required-projects list in Zuul, we need to ensure they are added here to ensure the tests pass on Zuul.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have relied on the required-projects list to install the collections used in our tests. Going forward, there's a possibility that the ansible.netcommon project might be removed from Zuul, as its tests no longer utilize Zuul for CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

From my understanding, collections specified in tests/integration/requirements.yml are not installed by Zuul.

Then, IMO, before we update the Zuul configs, that's what we should be fixing. There is a need to separate the run time dependencies from the test dependencies. Just because we install something to make our lives easier when building our test cases shouldn't mean we force customers to install them for run-time it's bad practice.

"ansible.utils": "*"
"amazon.cloud": "*"
"ansible.netcommon": "*"
"community.crypto": "*"
"community.aws": "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"community.aws": "*"

In theory amazon.aws can be used on its own

"community.general": "*"
documentation: https://ansible-collections.github.io/amazon.aws/branch/main/collections/amazon/aws/index.html
homepage: https://github.com/ansible-collections/amazon.aws
issues: https://github.com/ansible-collections/amazon.aws/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc
Expand Down
1 change: 1 addition & 0 deletions plugins/modules/ec2_vpc_route_table.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@
# Note: These examples do not set authentication details, see the AWS Guide for details.

# Basic creation example:
# Testing
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this can be removed.

- name: Set up public subnet route table
amazon.aws.ec2_vpc_route_table:
vpc_id: vpc-1245678
Expand Down
4 changes: 4 additions & 0 deletions tests/integration/requirements.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,7 @@ collections:
- ansible.windows
- ansible.utils # ipv6 filter
- amazon.cloud # used by integration tests - rds_cluster_modify
- ansible.netcommon
Copy link
Contributor

Choose a reason for hiding this comment

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

to be consistent with the Zuul configuration, the dependency should be defined using:

- name: https://github.com/ansible-collections/{{ namespace }}.{{ name }}.git
  type: git
  version: main

- community.crypto
- community.aws
Copy link
Contributor

Choose a reason for hiding this comment

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

For testing amazon.aws (and for testing community.aws, amazon.aws) this needs to come directly from git, not from galaxy. Otherwise we can't coordinate breaking changes between the two repos.

Copy link
Contributor

Choose a reason for hiding this comment

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

@GomathiselviS community.aws should not be defined here, it should be kept in the required project on the integration job because we are testing both collections with the same branch (for instance stable-X on will be tested with community.aws:Stable-X)

- community.general
Loading