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

Allow additional collections in only-builtins #2710

Merged
merged 1 commit into from Nov 23, 2022

Conversation

evgeni
Copy link
Contributor

@evgeni evgeni commented Nov 22, 2022

No description provided.

@evgeni evgeni requested review from a team as code owners November 22, 2022 13:18
@evgeni evgeni force-pushed the only-allow branch 2 times, most recently from a0c86cb to f133495 Compare November 22, 2022 13:37
@evgeni
Copy link
Contributor Author

evgeni commented Nov 22, 2022

I do not exactly understand why it fails on MacOS.

@ssbarnea ssbarnea changed the title allow additional collections in only-builtins Allow additional collections in only-builtins Nov 22, 2022
@evgeni
Copy link
Contributor Author

evgeni commented Nov 22, 2022

@ssbarnea can/will you investigate the macOS errors, or should I try to? (I don't have macOS here, so will have to fall back to abusing GH actions 😿 )

@ssbarnea
Copy link
Member

@ssbarnea can/will you investigate the macOS errors, or should I try to? (I don't have macOS here, so will have to fall back to abusing GH actions 😿 )

The bad news is that I do have MacOS and I am not able to reproduce the error locally. Feel free to abuse them as much as you need.

@evgeni evgeni force-pushed the only-allow branch 2 times, most recently from cbbbfc6 to d8c312d Compare November 23, 2022 08:14
@evgeni
Copy link
Contributor Author

evgeni commented Nov 23, 2022

Okay, so the error on macOS is:

syntax-check[specific]: couldn't resolve module/action 'fake_namespace.fake_collection.fake_module'. This often indicates a misspelling, missing collection, or incorrect module path.

I don't exactly understand why it resolves fine on Linux but not on macOS, but I will try to explicitly add it to the mocked list in the config.

Edit: okay, I am 100% puzzled by the configuration loader.

  • in test_only_builtin_fail we explicitly use --config-file=/dev/null yet the tests passes correctly -- I bet it fails on syntax tho, not on only-builtins and only passes because assert "only-builtins" in result.stdout also matches the filename 🤦‍♀️ -- I've split out the fix for that into Fix only builtins fail test #2719
  • test_only_builtins_allow_collections should have also failed on Linux, as the "old" config didn't contain any mock_modules and the global .ansible-lint that has it set should not be loaded (as we passed a custom config)

@evgeni evgeni force-pushed the only-allow branch 4 times, most recently from 452f9cc to a2b3271 Compare November 23, 2022 09:50
@evgeni
Copy link
Contributor Author

evgeni commented Nov 23, 2022

This is now green and ready. I still don't understand why it didn't fail on Linux (IMHO it should), but I also don't strictly care, now that it has been taken care of.

@ssbarnea ssbarnea merged commit 5b14a2b into ansible:main Nov 23, 2022
@evgeni evgeni deleted the only-allow branch November 23, 2022 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants