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

Tell pants about tests that use globs to test with many pack fixtures #5874

Merged
merged 14 commits into from
Feb 3, 2023

Conversation

cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Jan 27, 2023

Background

This is another part of introducing pants, as discussed in various TSC meetings.

Related PRs can be found in:

Overview of this PR

This PR:

  • adds a packs_glob target (in pants-plugins/pack_metadata) with a rule that "infers" dependencies (like a * glob) on all of the subdirectories (unless ignored in the dependencies field).
  • creates a python fixture file in st2tests/st2tests/fixtures/packs/all_packs_glob for tests that use globs to discover a bunch of packs. This fixture depends on the packs_glob(name="all_packs") target to facilitate the glob behavior.

This PR is basically a follow-up to the "Use python imports to identify fixtures" PRs: #5699, #5702, #5703, #5704, #5705, and #5706. But, instead of including these fixtures in that series of PRs, I waited to add this until after the pack_metadata targets were added in #5871.

@cognifloyd cognifloyd added this to the pants milestone Jan 27, 2023
@cognifloyd cognifloyd self-assigned this Jan 27, 2023
@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Jan 27, 2023
@cognifloyd cognifloyd changed the title Pants pack globs Use python imports to tell pants about tests that use globs to test with many pack Jan 27, 2023
@cognifloyd cognifloyd enabled auto-merge (squash) January 27, 2023 21:34
@cognifloyd cognifloyd changed the title Use python imports to tell pants about tests that use globs to test with many pack Use python imports to tell pants about tests that use globs to test with many packs Jan 27, 2023
@cognifloyd cognifloyd enabled auto-merge (squash) January 27, 2023 21:34
Copy link
Contributor

@amanda11 amanda11 left a comment

Choose a reason for hiding this comment

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

I am worried that we are going to forget to do this when we add a new test pack.
I am just wondering if we can either discover these or if we need to have a comment in the current test packs that exist that they are registered here. So that if someone copies an existing pack they will see the message that they also have to add here.

@cognifloyd
Copy link
Member Author

cognifloyd commented Jan 29, 2023

Yes. I agree. I already stumbled over that when packs were added after I added this (in my PoC branch awhile ago). So, I want a better solution than just comments.

Dependencies are not allowed to use globs because things like import * are generally a bad idea and make dependency inference much more complicated.

But, I can probably add a new pants plugin, or perhaps extend the pack_metadata plugin, with a packs_glob target. Then I would inject dependencies on all the globbed pack directories. 🤔

Yeah. I'll add it to pack_metadata and see how that works.

@cognifloyd cognifloyd marked this pull request as draft January 30, 2023 16:54
auto-merge was automatically disabled January 30, 2023 16:54

Pull request was converted to draft

@cognifloyd
Copy link
Member Author

Upon closer inspection, I realized I don't understand the one test that would use the child_packs fixture, and it's not really using a glob like the all_packs one, so I removed it.

Also, I updated target(name="all_packs") to use be a new packs_glob target instead. This way we don't have to manually register all of the packs. As soon as I document that in the PR description, I will mark this ready for review again.

@cognifloyd cognifloyd marked this pull request as ready for review January 30, 2023 19:00
@cognifloyd cognifloyd requested review from amanda11 and a team January 30, 2023 19:25
@cognifloyd cognifloyd changed the title Use python imports to tell pants about tests that use globs to test with many packs Tell pants about tests that use globs to test with many pack fixtures Jan 30, 2023
@cognifloyd
Copy link
Member Author

This is ready for review now.

Comment on lines +1 to +3
packs_glob(
name="all_packs",
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing depends on this (and the one in packs_invalid) directly right now. We will need both to include all the packs in the wheel.

@cognifloyd cognifloyd merged commit f71beb6 into master Feb 3, 2023
@cognifloyd cognifloyd deleted the pants-pack-globs branch February 3, 2023 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement pantsbuild size/L PR that changes 100-499 lines. Requires some effort to review. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants