-
Notifications
You must be signed in to change notification settings - Fork 95
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
Support additional target types #226
Conversation
The following target types are added: - compound - grain_pcre - ipcidr - pillar - pillar_exact - pillar_pcre - range - pcre
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey thanks for this Contribution. The PR looks good in general i just added some comments for things that could be simplified a bit.
* | ||
* @param <T> The data type of the target class | ||
*/ | ||
abstract class AbstractTarget<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not also take the TargetType
as parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! I'll change that.
|
||
protected final DictionaryTargetExpression expr; | ||
|
||
public DictionaryTarget(DictionaryTargetExpression expr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
|
ae9c0f0
to
1dcdbaf
Compare
I've made all the requested changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.