Support additional target types #226
Conversation
The following target types are added: - compound - grain_pcre - ipcidr - pillar - pillar_exact - pillar_pcre - range - pcre
| * | ||
| * @param <T> The data type of the target class | ||
| */ | ||
| abstract class AbstractTarget<T> { |
There was a problem hiding this comment.
AbstractTarget seems to be similar to Target i think we could get rid of the interface and rename the AbstractTarget to Target.
There was a problem hiding this comment.
Each target type expresses its compatibility with the local client or ssh client with a declaration like this:
public class Glob extends AbstractTarget<String> implements Target<String>, SSHTarget<String>I'm not sure that would continue to work if we change the interfaces to base classes. Can you suggest a solution?
There was a problem hiding this comment.
Right. This does not seem that easy after all. I think its ok to leave it like that for now. 👍
|
|
||
| protected final T target; | ||
|
|
||
| protected AbstractTarget(T target) { |
There was a problem hiding this comment.
Why not also take the TargetType as parameter?
There was a problem hiding this comment.
Great idea! I'll change that.
|
|
||
| protected final DictionaryTargetExpression expr; | ||
|
|
||
| public DictionaryTarget(DictionaryTargetExpression expr) { |
There was a problem hiding this comment.
DictionaryTarget and DictionaryTargetExpression seems a bit much i wonder if we could just move all the functionality in DictionaryTarget.
There was a problem hiding this comment.
That is what I had in 40efc77. I moved to DictionaryTargetExpression because I found I was duplicating a lot of code. You have 5 types that all take the same expression format, and each of them has 4 constructors. So I had 20 constructors that just called the base class constructor with the same arguments, and duplicated tests for all of these as well.
Is there a better way to solve that?
There was a problem hiding this comment.
I don't see a better way to solve that but at least those constructors won't have any logic in them so not much to get wrong. Its annoying to have this kind of boilerplate but i think its worth it if it improves usability of the API.
| */ | ||
| @Deprecated | ||
| public Grains(String grain, String value) { |
There was a problem hiding this comment.
Having a constructor like this is more convenient to use then having to create a DictionaryTargetExpression.
|
Hey @amendlik, thanks again for contributing! There is a checkstyle error that was introduced with your latest commit: |
amendlik
force-pushed
the
target-types
branch
from
ae9c0f0
to
1dcdbaf
Compare
|
I've made all the requested changes. |
amendlik
force-pushed
the
target-types
branch
from
2589414
to
4ea42fc
Compare
|
Hey thanks again for the contribution. I just merged your PR so it will be in the next release but until then you have to use a SNAPSHOT release if you want to work with your changes. 👍 |
This PR addresses #103 by adding support for the following target types:
It also adds support for special delimiters (other than
:) in grain and pillar targets, and their variants.