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

Add subsystem for appending tags to targets from a single source #7315

Merged
merged 3 commits into from Mar 7, 2019

Conversation

Projects
None yet
5 participants
@codealchemy
Copy link
Contributor

commented Mar 5, 2019

Problem

It can be cumbersome to apply the same tags to many targets as it requires editing every associated BUILD file. Having a single source of tags would make it more straightforward to configure tags by which to filter targets (#6902), and can be applied to other use cases such as grouping targets with a particular suffix (ex. _integration in pants) under a single tag.

Solution

Add a subsystem within Target responsible for fetching, parsing, and matching tags configured in a JSON file to targets. The subsystem matches against the target's address (though could be expanded to a regex to address additional use cases).

Closes #7302

@stuhood

stuhood approved these changes Mar 5, 2019

Copy link
Member

left a comment

Applying tags in the Target constructor will only work with v1, but the conversation on #7302 shows that it is clearly not easy enough yet to do this in v2. I've opened #7316 about that.

@classmethod
def register_options(cls, register):
register('--tag-targets-file', type=str, default=None, fingerprint=True,
help='Source JSON file with tag assignments for targets. Ex: \

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 5, 2019

Member

Rather than JSON (which doesn't support comments), it might be preferable have this be a type=dict option, marked fromfile=True.

That way folks could define the value either directly inline in pants.ini, or in a separate file via @fromfile.

Apparently fromfile=True isn't documented here: https://www.pantsbuild.org/options.html#dict-options ... but dict options are at least.

This comment has been minimized.

Copy link
@codealchemy

codealchemy Mar 6, 2019

Author Contributor

Sounds good - I started down that path and got hung up on getting the fromfile option under tests, will push an update once I get that resolved

This comment has been minimized.

Copy link
@codealchemy

codealchemy Mar 6, 2019

Author Contributor

I added d4e53c7 without testing the fromfile option explicitly as there are other tests to show that fromfile works. I can come back around to adding coverage if needed.

@baroquebobcat
Copy link
Contributor

left a comment

This looks like it's a good start.

Could you add a short documentation page that shows how this file would be used? I think it'd be nice if we had a page on tags and how they're used in general, and showing this could be a good starting point for that.

For example, something that shows a couple targets with tags, how to select for tags on the commandline, and how an example file for this could be used to centrally add tags to targets.

self.assertEqual({'tag2', 'special_tag', 'special_tag2'}, target3.tags)
self.assertEqual({'tag3', 'special_tag2'}, target4.tags)
self.assertEqual({'tag3'}, target5.tags)

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Mar 5, 2019

Contributor

Things that might be nice to test

  • what happens when a target has the special tag already (probably nothing)
  • what happens when the tag target mapping points to a non-existent target (probably ignore it--differentiating non-existent vs not in the current closure is probably something the subsystem shouldn't worry about)
  • if an invalidly formatted spec is in the source file, what does the error look like (It'd be nice if the error pointed to the JSON file)

This comment has been minimized.

Copy link
@codealchemy

codealchemy Mar 6, 2019

Author Contributor

what happens when a target has the special tag already (probably nothing)

Nothing, I included target5 to show nothing changes if things don't match, but can add more cases if it's helpful

what happens when the tag target mapping points to a non-existent target (probably ignore it--differentiating non-existent vs not in the current closure is probably something the subsystem shouldn't worry about)

It's ignored, added a case for that in the test - though that illustrates that it's possible to define things in the tag mappings that don't do anything, not sure if it'd be preferable to maybe raise an error if something is defined that doesn't match

if an invalidly formatted spec is in the source file, what does the error look like (It'd be nice if the error pointed to the JSON file)

Just switched it to use a dict and fromfile instead of JSON, tried things out with a malformed JSON file and a ParseError was raised

@illicitonion

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

FWIW I prefer scripting to edit build files, rather than having multiple sources of truth that get merged into targets... It makes it easier for someone reading a BUILD file to understand what it means, rather than needing to know some magical locations that may change the meaning of a target.

Assuming you're using lists for your tags, buildozer 'add tags foo bar' some:target some/other:target (using buildozer) will edit the appropriate BUILD files to just add the right tags. buildozer 'remove tags foo' some:target will again do the right thing.

I don't believe buildozer currently supports editing set literals (in case you're using sets not lists), but it would be pretty trivial to extend it to do so... (bazelbuild/buildtools#146 added set support to the parser, as a jumping off point)

codealchemy added some commits Mar 6, 2019

Leverage fromfile option to simplify providing tag / targets mappings
The keys / values for this option should parse correctly to a python dict from json (no booleans or nulls), so adding this allows for either a source file as before, or a dict added to the pants.ini
Fixes from linter feedback
Missed in the switch to using fromfile for the tag assignment subsystem
@codealchemy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Re:

Could you add a short documentation page that shows how this file would be used? I think it'd be nice if we had a page on tags and how they're used in general, and showing this could be a good starting point for that.

👍 can do! Though feels like that might fit best as a followup PR to the changes here.

@benjyw

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

@illicitonion If people want to do that they can, of course. But the request for central target whilelist/blacklist support has been longstanding, and it seems like a reasonable thing to have.

@benjyw

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Also, this still works in a glorious future in which we infer target definitions and don't have BUILD files at all...

@illicitonion

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

@illicitonion If people want to do that they can, of course. But the request for central target whilelist/blacklist support has been longstanding, and it seems like a reasonable thing to have.

Yeah, I'm definitely not going to block this feature, just noting that operationally and for users it can be a source of confusion :)

@stuhood

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

One other thing that is worth calling out here: almost anything that this subsystem will want to do is currently implemented by the filter task. I wonder whether there is an elegant unification of the two?

We've discussed the fact that many things on filter could in theory become global options (ie ./pants --target-type=junit_test test ::, for example). And the counter argument has been: "we should lean in and create a general way to query" (see #6501).

I'm still not certain I'm in favor of going all the way to "query language", but it's worth thinking holistically about whether we can use this system to reduce the total number of filtering concepts in the system.

@benjyw benjyw merged commit afa0e16 into pantsbuild:master Mar 7, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@codealchemy codealchemy deleted the codealchemy:apply-tags-from-json branch Mar 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.