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

Fix requires_ansible check when no meta/runtime.yml #124

Merged
merged 1 commit into from
Jun 24, 2021
Merged

Fix requires_ansible check when no meta/runtime.yml #124

merged 1 commit into from
Jun 24, 2021

Conversation

awcrosby
Copy link
Contributor

Fail import when there is no meta/runtime.yml

Fixes a missed case from #122 and AAH-538

No-Issue

alikins
alikins reviewed Jun 24, 2021
View reviewed changes
tests/unit/test_collection.py
@@ -77,7 +77,7 @@
"name": "FILES.json",
"ftype": "file",
"chksum_type": "sha256",
"chksum_sha256": "dc90402feea54f479780e067cba748559cb01bff52e6724a15264b9a55e8f000",
"chksum_sha256": "0c2e9514f54aaeda0605d8c9a60e71cab3e577113b58ecc65b839d2d1b86c216",
Copy link
Contributor

Choose a reason for hiding this comment

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

<general comment on existing code, not a requested change...>

Updating the tests to calculate this on the fly might be useful in the future. I didn't do it in an attempt to stay focused on the feature. Maybe it's better as is for explicitness, I'm not sure. It is a tiny bit annoying to update though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haven't had to update this much so should be ok, and wasn't a bad update

@awcrosby awcrosby force-pushed the fix/fail_if_no_meta_runtime branch from 76e57bb to 461a712 Compare June 24, 2021 19:24
@awcrosby awcrosby requested a review from alikins June 24, 2021 19:27
alikins
alikins approved these changes Jun 24, 2021
View reviewed changes
Copy link
Contributor

@alikins alikins left a comment

Choose a reason for hiding this comment

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

LGTM. Small suggestion for updating the errmsg for the empty (or False-y) contents of runtime.yml, but that is not a blocker.

galaxy_importer/loaders.py Outdated
@@ -193,7 +193,7 @@ def _load(self):

def get_requires_ansible(self):
if not self.data:
return
raise exc.RuntimeFileError("'requires_ansible' in meta/runtime.yml is mandatory")
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably fine. Maybe tweak the error msg to indicate the contents of meta/runtime.yml was empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@awcrosby
Fix requires_ansible check when no meta/runtime.yml
c7e3b1b 
Fail import when there is no meta/runtime.yml

No-Issue
@awcrosby awcrosby force-pushed the fix/fail_if_no_meta_runtime branch from 461a712 to c7e3b1b Compare June 24, 2021 20:53
@awcrosby awcrosby merged commit 7521612 into ansible:master Jun 24, 2021
@awcrosby awcrosby deleted the fix/fail_if_no_meta_runtime branch June 24, 2021 20:57
@felixfontein
Copy link
Contributor

@awcrosby @alikins neither meta/runtime.yml nor requires_ansible are mandatory for collections. Why do you require them?

This was referenced Jul 29, 2021
Closed
Closed
sshnaidm added a commit to sshnaidm/ansible that referenced this pull request Jul 29, 2021
@sshnaidm
Add packaging to requirement of ansible-test
347c9e9 
Fix ansible#75353
After requires_ansible field was added as mandatory to runtime.yml
file, ansible-test fails to check this field if it doesn't have
packaging module.

[1] ansible/galaxy-importer#124
@sshnaidm sshnaidm mentioned this pull request Jul 29, 2021
Merged
mattclay pushed a commit to ansible/ansible that referenced this pull request Jul 29, 2021
@sshnaidm
Add packaging to requirement of ansible-test (#75356)
40ca87a 
* Add packaging to requirement of ansible-test

Fix #75353

After requires_ansible field was added as mandatory to runtime.yml
file, ansible-test fails to check this field if it doesn't have
packaging module.

[1] ansible/galaxy-importer#124
mattclay pushed a commit to mattclay/ansible that referenced this pull request Jul 29, 2021
@sshnaidm @mattclay
[stable-2.11] Add packaging to requirement of ansible-test (ansible#7…
dc607cb 
…5356)

* Add packaging to requirement of ansible-test

Fix ansible#75353

After requires_ansible field was added as mandatory to runtime.yml
file, ansible-test fails to check this field if it doesn't have
packaging module.

[1] ansible/galaxy-importer#124
(cherry picked from commit 40ca87a)

Co-authored-by: Sergey <sshnaidm@users.noreply.github.com>
@mattclay mattclay mentioned this pull request Jul 29, 2021
Merged
openstack-mirroring pushed a commit to openstack/openstack that referenced this pull request Jul 29, 2021
@openstack-gerrit
Update git submodules
ee118d2 
* Update tripleo-operator-ansible from branch 'master'
  to 1405de60175b8cf28dfa4950da6536486ada80ed
  - Merge "Add meta/runtime.yml file required by galaxy"
  - Add meta/runtime.yml file required by galaxy
    
    Ansible Galaxy added[1][2] a mandatory requirement for meta/runtime.yml
    file and requires_ansible field in it. Add it to allow uploading
    to Galaxy.
    
    [1] ansible/galaxy-importer#122
    [2] ansible/galaxy-importer#124
    
    Change-Id: If925bb36c7722e8e5e150eb7f5ce9e6316fd9539
openstack-mirroring pushed a commit to openstack-archive/tripleo-operator-ansible that referenced this pull request Jul 29, 2021
@sshnaidm
Add meta/runtime.yml file required by galaxy
5a14392 
Ansible Galaxy added[1][2] a mandatory requirement for meta/runtime.yml
file and requires_ansible field in it. Add it to allow uploading
to Galaxy.

[1] ansible/galaxy-importer#122
[2] ansible/galaxy-importer#124

Change-Id: If925bb36c7722e8e5e150eb7f5ce9e6316fd9539
mattclay pushed a commit to ansible/ansible that referenced this pull request Jul 30, 2021
@sshnaidm @mattclay
[stable-2.11] Add packaging to requirement of ansible-test (#75356)
da1baaf 
* Add packaging to requirement of ansible-test

Fix #75353

After requires_ansible field was added as mandatory to runtime.yml
file, ansible-test fails to check this field if it doesn't have
packaging module.

[1] ansible/galaxy-importer#124
(cherry picked from commit 40ca87a)

Co-authored-by: Sergey <sshnaidm@users.noreply.github.com>
meffie added a commit to openafs-contrib/buildbot that referenced this pull request Jul 31, 2021
@meffie
ansible-galaxy now requires meta/runtime.yml
d4b29c7 
Add the meta/runtime.yml to appease galaxy imports.

See ansible-collections/overview#45

<quote>
Ansible Galaxy unexpectedly made a change to require mandatory
requires_ansible in meta/runtime.yml file for uploading
collection[1][2].  And after you add it you'll probably will hit
ansible-test failure in sanity checks if you use it with --venv option,
since it can't check this field because of failing requirement[3].
Worth to mention you must have meta/runtime.yml now for your collection
to be uploaded.

[1] ansible/galaxy-importer#122
[2] ansible/galaxy-importer#124
[3] ansible/ansible#75353
</quote>
meffie added a commit to openafs-contrib/ansible-openafs that referenced this pull request Aug 2, 2021
@meffie
ansible-galaxy now requires meta/runtime.yml
da4e78d 
Add the meta/runtime.yml to appease galaxy imports.

See ansible-collections/overview#45

<quote>
Ansible Galaxy unexpectedly made a change to require mandatory
requires_ansible in meta/runtime.yml file for uploading
collection[1][2].  And after you add it you'll probably will hit
ansible-test failure in sanity checks if you use it with --venv option,
since it can't check this field because of failing requirement[3].
Worth to mention you must have meta/runtime.yml now for your collection
to be uploaded.

[1] ansible/galaxy-importer#122
[2] ansible/galaxy-importer#124
[3] ansible/ansible#75353
</quote>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alikins alikins alikins approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@awcrosby @felixfontein @alikins