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.

Sign up for GitHub

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

Jump to bottom

Add App Search extension #471

Merged
merged 15 commits into from
Mar 5, 2020
Merged

Add App Search extension #471

merged 15 commits into from
Mar 5, 2020

Conversation

jwsy
Copy link
Contributor

@jwsy jwsy commented Mar 2, 2020

Ref #470 - this is a PR to add App Search as an extension as recommended by @antoineco (great instructions, thank you)!

I don't know how you'd like to have me add test cases, but a straightforward one seems to use the given App Search credentials passed as environment variables in the compose yml. I've added those instructions to the README as well as a note about how to update root directory docker-compose.yml to use the Elasticsearch configs included in this extension.

# PASS 
$ curl 'http:https://localhost:3002/as/' -u app_search:changeme
<!DOCTYPE html5><html lang="en"><head><title>App Search</title><meta name="csrf-param" content="authenticity_token" />
<meta name="csrf-token" content="zlqJdX5Jc9ZSqhVnxanIJsB8s6bx9cYzvVb/1u6yokboJGALvIXbRwb/IsOYJJkZ3NePNUf/VYd245D6shE1qA=="
... Lots more data

# FAIL
$ curl 'http:https://localhost:3002/as/' -u app_search:WRONG_PASSWORD
<html><body>You are being <a href="http:https://localhost:3002/login">redirected</a>.</body></html>%```

antoineco
antoineco requested changes Mar 2, 2020
View reviewed changes
Copy link
Collaborator

@antoineco antoineco left a comment

Choose a reason for hiding this comment

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

Looking good! I only have a few comments.

.env Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
extensions/app-search/config/elasticsearch.yml Outdated Show resolved Hide resolved
extensions/app-search/README.md Outdated Show resolved Hide resolved
extensions/app-search/config/app-search.yml Outdated Show resolved Hide resolved
@antoineco
Copy link
Collaborator

antoineco commented Mar 2, 2020 •
edited
Loading

One more comment: I found an example of configuration for Docker Compose in App Search's official docs, and this example is functional with a very minimal Elasticsearch configuration.

If the configuration of authentication realms and indexes is optional, I'd rather stick to the defaults and let users fine tune their setup themselves.

All services in the stack should be as unopinionated as possible to guarantee a neutral starting point for everyone.

@jwsy
Copy link
Contributor Author

jwsy commented Mar 4, 2020

Updates made, thank you very much for your great examples & guidance @antoineco !

@antoineco
Copy link
Collaborator

@jwsy Thank you for the good work!

I'll push a commit or two to your PR in order to include some tests that we can run in Travis CI, and then we should be ready to merge.

@antoineco
Copy link
Collaborator

antoineco commented Mar 4, 2020 •
edited
Loading

@jwsy the App Search tests are green. I also made some cosmetic changes to the README so that it's more aligned with the main documentation.

I'll merge as soon as you confirm you're satisfied with the current state.

@jwsy
Copy link
Contributor Author

jwsy commented Mar 5, 2020

More than satisfied with the current state (excellent improvement to the docs) and thank you very much for being a great example, @antoineco!

I appreciate the excellent direction you've provided me, and thank you for being a wonderful steward for this project that has been very helpful to many.

@antoineco
Copy link
Collaborator

🙌

@antoineco antoineco merged commit 27bb58d into deviantony:master Mar 5, 2020
antoineco added a commit that referenced this pull request Mar 5, 2020
@antoineco
Add App Search extension (#471)
149cc76 
Co-authored-by: Antoine Cotten <[email protected]>
antoineco added a commit that referenced this pull request Mar 5, 2020
@jwsy @antoineco
Add App Search extension (#471)
2d90bfc 
Co-authored-by: Antoine Cotten <[email protected]>
DanBrown47 pushed a commit to DanBrown47/docker-elk that referenced this pull request Jun 22, 2023
@jwsy @antoineco
Add App Search extension (deviantony#471)
2737dc7 
Co-authored-by: Antoine Cotten <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoineco antoineco antoineco approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jwsy @antoineco