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

galaxy.yml: exclude non-collection files from collection build #3517

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dmsimard
Copy link
Contributor

@dmsimard dmsimard commented Oct 5, 2021

SUMMARY

Files that belong to the git repository such as git, github and
azure pipeline configuration should not be included in the published
collection tarball.

Excluding tests saves on what makes the bulk of the size of the
collection, both in size and number of files.

In aggregate, excluding these files results in a meaningful improvement
which will speed up the installation of the collection and reduce the
size on disk.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

galaxy.yml

ADDITIONAL INFORMATION

Before

> du -sh community-general-3.7.0.tar.gz 
2.2M community-general-3.7.0.tar.gz

> du -sh community.general
18M	community.general

> find . -type f | wc -l
2127

> time ansible-galaxy collection install /home/dmsimard/Downloads/community-general-3.7.0.tar.gz 
Starting galaxy collection install process
Process install dependency map
Starting collection install process
Installing 'community.general:3.7.0' to '/home/dmsimard/.ansible/collections/ansible_collections/community/general'
community.general:3.7.0 was installed successfully
________________________________________________________
Executed in   10.44 secs    fish           external
   usr time    9.91 secs    0.00 micros    9.91 secs
   sys time    0.35 secs  606.00 micros    0.35 secs

After

> du -sh community-general-3.7.0.tar.gz 
1.6M community-general-3.7.0.tar.gz

> du -sh community.general
9.1M community.general

> find . -type f | wc -l
738

> time ansible-galaxy collection install /home/dmsimard/dev/git/ansible_collections/community/general/community-general-3.7.0.tar.gz
Starting galaxy collection install process
Process install dependency map
Starting collection install process
Installing 'community.general:3.7.0' to '/home/dmsimard/.ansible/collections/ansible_collections/community/general'
community.general:3.7.0 was installed successfully
________________________________________________________
Executed in    6.75 secs    fish           external
   usr time    6.43 secs    0.00 micros    6.43 secs
   sys time    0.20 secs  614.00 micros    0.20 secs

Files that belong to the git repository such as git, github and
azure pipeline configuration should not be included in the published
collection tarball.

Excluding tests saves on what makes the bulk of the size of the
collection, both in size and number of files.

In aggregate, excluding these files results in a meaningful improvement
which will speed up the installation of the collection and reduce the
size on disk.
@dmsimard dmsimard marked this pull request as draft October 5, 2021 20:56
@ansibullbot ansibullbot added WIP Work in progress bug This issue/PR relates to a bug labels Oct 5, 2021
@dmsimard
Copy link
Contributor Author

dmsimard commented Oct 5, 2021

Marked as WIP while we discuss ansible-community/community-topics#29

- .azure-pipelines
- .github
- .gitignore
- changelogs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- changelogs

It is absolutely essential that changelogs/changelog.yaml is included in the build. Otherwise building the Ansible changelog will not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you expand on that ? I was under the impression that leaving CHANGELOG.rst was sufficient ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

changelogs/changelog.yaml is the source of truth, both for CHANGELOG.rst (included in the collection) and the Ansible changelog are generated from it. The .rst file is only for the convenience of users and maintainers as it's easier to read, and also can have any other name or location.

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Adding a negative review to prevent accidental merging. There are some legal questions that need to be resolved first before this can be merged (next to the necessary change w.r.t. changelog).

@gotmax23
Copy link
Contributor

gotmax23 commented Aug 2, 2022

Has there been any progress on this? What are the legal issues here?

@felixfontein
Copy link
Collaborator

There is no progress (to my knowledge). The legal problem still is (if I remember correctly): the collection tarball acts both as the source and the installable artefact. Excluding files used for the development from it would violate the GPL.

@bcoca
Copy link
Contributor

bcoca commented Aug 2, 2022

@felixfontein IANAL but even including a URI to those files should be sufficient, the files and any changes must be available, but it does not mean every possible form of distribution of the product must have them ... otherwise distributing binary packages would be untenable.

@felixfontein
Copy link
Collaborator

I'm not sure what the exact problems are, the community team has been in (non-?)communication with RH legal on this matter for ... quite a long time now. I don't know exactly what they asked, but so far there was no answer AFAIK.

@gotmax23
Copy link
Contributor

I've created ansible-community/community-topics#126 to discuss this.

@dmsimard
Copy link
Contributor Author

I'm not sure what the exact problems are, the community team has been in (non-?)communication with RH legal on this matter for ... quite a long time now. I don't know exactly what they asked, but so far there was no answer AFAIK.

This is a fair assessment -- I do not remember when because it's been so long now and I'm no longer employed at Red Hat to check my emails but I opened a ticket with Red Hat open source legal describing this issue and asking for advice but have not heard back.

There is a ticket number somewhere if someone would like to follow up internally but I do not know what it is.
It is possible that @gundalow could be able to find it.

@felixfontein
Copy link
Collaborator

Looks like we can continue with this, according to ansible-community/community-topics#131. I would still keep the .md files, and changelogs definitely must stay.

@felixfontein felixfontein added breaking_change This PR contains a breaking change that MUST NOT be backported check-before-release PR will be looked at again shortly before release and merged if possible. labels Nov 23, 2022
@felixfontein
Copy link
Collaborator

Also this needs a changelog fragment. IMO this is a breaking change and has to wait for the next major release. (Someone could depend on importing from ansible_collections.community.general.tests.unit for example.)

@gotmax23
Copy link
Contributor

The list I use for the Fedora ansible-collection-community-general package is:

  • .azure-pipelines
  • .github
  • .gitignore
  • .pre-commit-config.yaml
  • changelogs/.gitignore
  • changelogs/fragments
  • tests

@dmsimard
Copy link
Contributor Author

Feel free to open a new PR, close this one and link to it for context.

I've been out of the loop for long enough and don't mind.

@webknjaz
Copy link
Member

Excluding tests saves on what makes the bulk of the size of the
collection, both in size and number of files.

Sounds like collections really need an sdist vs a wheel separation too...

@gotmax23
Copy link
Contributor

Excluding tests saves on what makes the bulk of the size of the
collection, both in size and number of files.

Sounds like collections really need an sdist vs a wheel separation too...

I strongly agree. The lack of collection source distributions presents a problem for the ansible package, because these files are removed from the ansible sdist if they are removed from the Galaxy artifact.

@felixfontein
Copy link
Collaborator

I also strongly agree (and I think I mentioned more than once during community meetings that this would be really helpful and solve most of our problems :) ).

@gotmax23
Copy link
Contributor

Sounds like collections really need an sdist vs a wheel separation too...

How can we move this forward? Should I create a community-topic? I guess we'd need agreement from the core team and the Galaxy team. IIRC, @jctanner mentioned that this would be difficult to implement with Galaxy NG's current architecture when we last discussed this.

@felixfontein
Copy link
Collaborator

@gotmax23 a community topic would be great. Another possible place would be https://github.com/ansible/proposals/, but since there should be something in https://github.com/ansible-community/community-topics/, even if just to coordinate everything, I guess we should start with that.

@gotmax23
Copy link
Contributor

I've opened ansible-community/community-topics#161

@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI and removed stale_ci CI is older than 7 days, rerun before merging labels Jul 24, 2023
@ansibullbot ansibullbot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Nov 29, 2023
@russoz
Copy link
Collaborator

russoz commented Oct 7, 2024

hi @dmsimard are you planning on working on this?

needs_info

@ansibullbot ansibullbot added the needs_info This issue requires further information. Please answer any outstanding questions label Oct 7, 2024
@dmsimard
Copy link
Contributor Author

dmsimard commented Oct 9, 2024

It is not on my to-do list.

It got caught up in the wheels of license compliance back when I was in the Red Hat community team and I didn't manage to hear back from legal before leaving.

Someone else can pick up this work if they want.

@ansibullbot ansibullbot removed the needs_info This issue requires further information. Please answer any outstanding questions label Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking_change This PR contains a breaking change that MUST NOT be backported bug This issue/PR relates to a bug check-before-release PR will be looked at again shortly before release and merged if possible. has_issue needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants