Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Added action for adding and deleting indexed scripts #213

Merged
merged 12 commits into from
Jun 29, 2015

Conversation

nitram509
Copy link
Contributor

I would like to add two more actions.
One adds indexed scripts, the other deletes them.

As a basis, I've used the reference at:
https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-scripting.html

Feedback is welcome.

Regards
Martin

@@ -69,6 +69,11 @@
</dependency>

<dependency>
<groupId>org.easytesting</groupId>
Copy link
Member

Choose a reason for hiding this comment

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

The assertions used (contains/null/equals) really fall short of justifying the need for this additional library as we already have junit. Let's stick with junit for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kramer
fest assertions works nicely together with JUnit and Hamcrest.
It offers a readable fluent API and meaningful assertion messages (if tests fails).
May I ask why stick with junit and not extending the possibilities of assertions?

Copy link
Member

Choose a reason for hiding this comment

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

I have nothing against fest I'm sure its as awesome as jest if not more 😄.

Main reason is consistency; having two test classes with a different assert syntax than the rest of the project is just awkward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.
I will rework to use JUnit instead of fest.

@kramer
Copy link
Member

kramer commented Jun 23, 2015

Also please feel free to add yourself to thanks section in readme :)

@nitram509
Copy link
Contributor Author

@kramer
I've worked on all the things we've talked about.

Would you please have a look at.
Do you miss something?
Feedback is always welcome ;-)

Regards
Martin

@kramer
Copy link
Member

kramer commented Jun 29, 2015

Just two very minor things:

  • I think you forgot to revert the pom file :).
  • Also forgive me for nitpicking but the list in Thanks section is alphabetically sorted 😊.

@nitram509
Copy link
Contributor Author

Fixed :-)

kramer pushed a commit that referenced this pull request Jun 29, 2015
Added actions for adding and deleting indexed scripts
@kramer kramer merged commit 5470a41 into searchbox-io:master Jun 29, 2015
@kramer
Copy link
Member

kramer commented Jun 29, 2015

Thank you for your contribution @nitram509 !

@nitram509 nitram509 deleted the script-update-action branch June 29, 2015 22:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants