-
Notifications
You must be signed in to change notification settings - Fork 338
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
Add dependent collections to galaxy.yml and requirements.yml #2145
Add dependent collections to galaxy.yml and requirements.yml #2145
Conversation
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 7m 59s |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 20m 31s |
@@ -93,6 +93,7 @@ | |||
# Note: These examples do not set authentication details, see the AWS Guide for details. | |||
|
|||
# Basic creation example: | |||
# Testing |
There was a problem hiding this comment.
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.
@@ -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": "*" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 25s |
@@ -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": "*" |
There was a problem hiding this comment.
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.
@@ -3,3 +3,7 @@ collections: | |||
- ansible.windows | |||
- ansible.utils # ipv6 filter | |||
- amazon.cloud # used by integration tests - rds_cluster_modify | |||
- ansible.netcommon |
There was a problem hiding this comment.
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
"amazon.cloud": "*" | ||
"ansible.netcommon": "*" | ||
"community.crypto": "*" | ||
"community.aws": "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"community.aws": "*" |
In theory amazon.aws can be used on its own
@@ -3,3 +3,7 @@ collections: | |||
- ansible.windows | |||
- ansible.utils # ipv6 filter | |||
- amazon.cloud # used by integration tests - rds_cluster_modify | |||
- ansible.netcommon | |||
- community.crypto | |||
- community.aws |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
)
I think the consensus here is that this change shouldn't be merged as is, these collections aren't needed for |
SUMMARY
Depends-On: ansible/ansible-zuul-jobs#1878
This PR adds dependent collections (needed for integration tests) to galaxy.yml and tests/integration/requirements.yml.
These collections can be removed from required-projects used in ansible-zuul-jobs.
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION