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

Events: Add placeholder to subqueries #19

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Dec 4, 2018

@bernhardreiter
Copy link
Member

Hi @wagner-certat,
thanks for the pull request.

I've had a first look,

  • To me it looks sensible to also add an example of a placeholder to the config file.
  • An example for using % as post and prefix for substring search would be illustrative.
  • The change introduces a dependency to intelmq which wasn't there before. Probably fine, maybe listed in setup.py so this gets an automatic check. (Before in principle, someone could run the module without having a full intelmq installed, e.g. on a pure database machine.)

What do you think?

Sebastian Wagner added 2 commits March 18, 2019 14:31
Add an example for the placeholder-usage in the subqueries
see PR Intevation#19
intelmq library is now required for the events_api
@ghost
Copy link
Author

ghost commented Mar 18, 2019

* To me it looks sensible to also add an example of a placeholder to the config file.

Good idea, done

* An example for using `%` as post and prefix for substring search would be illustrative.

Also added

* The change introduces a dependency to `intelmq`  which wasn't there before. Probably fine, maybe listed in setup.py so this gets an automatic check. (Before in principle, someone could run the module without having a full intelmq installed, e.g. on a pure database machine.)

It actually does only require the installed library, not the full (running) application. I didn't even check if the requirement is already there because I assumed it is. Thanks for the hint, I added it.

@ghost
Copy link
Author

ghost commented Oct 22, 2019

Resolved the conflicts with master

@ghost
Copy link
Author

ghost commented Nov 22, 2019

Is there interest in this feature?

@bernhardreiter
Copy link
Member

@wagner-certat yes so far I think it is a useful feature. (We currently lack contracted time to properly review it.)

BTW: I wonder if the intelmq dependency could be made optional with little effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant