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

feat/13-implement-search #320

Merged
merged 1 commit into from
Dec 1, 2022
Merged

feat/13-implement-search #320

merged 1 commit into from
Dec 1, 2022

Conversation

reglim
Copy link
Contributor

@reglim reglim commented Oct 20, 2022

I created an index database that gets updated at every modification and is used to speed up searching projects, versions, tags and content. I also added the actual search page itself and a search bar in the header.

image

Can someone please verify that the docker pass-through for the index.json database should work? I don't have much experience with that.

fixes #13

@fliiiix
Copy link
Member

fliiiix commented Oct 26, 2022

I guess we need to rethink how we do things im not a fan of how many things one needs to configure

--volume $PWD/docat-run/doc:/var/docat/doc/ \
  --volume $PWD/docat-run/db:/var/docat/db/ \
  --env DOCAT_DB_PATH=/var/docat/db/db.json
  --env DOCAT_INDEX_PATH=/var/docat/db/index.json

@reglim
Copy link
Contributor Author

reglim commented Oct 26, 2022

I know it's not nice but we need to save the index somehow because indexing is really expensive. What we could do is just provide one DB_PATH variable that is used for both databases.

reglim added a commit that referenced this pull request Oct 31, 2022
@reglim reglim force-pushed the Feat/13-Add-Search branch from 54b638c to b3d5bd8 Compare October 31, 2022 07:01
reglim added a commit that referenced this pull request Oct 31, 2022
@reglim reglim force-pushed the Feat/13-Add-Search branch from b3d5bd8 to cfe32c9 Compare October 31, 2022 07:13
reglim added a commit that referenced this pull request Oct 31, 2022
@reglim reglim force-pushed the Feat/13-Add-Search branch from cfe32c9 to c77c796 Compare October 31, 2022 07:19
reglim added a commit that referenced this pull request Oct 31, 2022
@reglim reglim force-pushed the Feat/13-Add-Search branch from c77c796 to ae3c745 Compare October 31, 2022 07:23
reglim added a commit that referenced this pull request Oct 31, 2022
@reglim reglim force-pushed the Feat/13-Add-Search branch from ae3c745 to d4ff2d2 Compare October 31, 2022 07:26
@reglim
Copy link
Contributor Author

reglim commented Oct 31, 2022

@fliiiix
Ok, I had to rebase the master and remove some typing.List references as well. Now we have one parameter:
What do you think of that?

# run container in background and persist data (docs, nginx configs and tokens database as well as the content index)
# use 'ghcr.io/docat-org/docat:unstable' to get the latest changes
mkdir -p docat-run/db && touch docat-run/db/db.json && touch docat-run/db/index.json
docker run \
  --detach \
  --volume $PWD/docat-run/doc:/var/docat/doc/ \
  --volume $PWD/docat-run/db/:/app/docat/ \
  --publish 8000:80 \
  ghcr.io/docat-org/docat

@reglim reglim force-pushed the Feat/13-Add-Search branch 2 times, most recently from 66c8f8e to 1fa2a21 Compare November 8, 2022 06:43
reglim added a commit that referenced this pull request Nov 15, 2022
@reglim reglim force-pushed the Feat/13-Add-Search branch from 1fa2a21 to b773b92 Compare November 15, 2022 15:35
@randombenj

This comment was marked as resolved.

docat/docat/app.py Outdated Show resolved Hide resolved
docat/docat/app.py Outdated Show resolved Hide resolved
docat/docat/app.py Outdated Show resolved Hide resolved
docat/docat/app.py Outdated Show resolved Hide resolved
docat/docat/app.py Outdated Show resolved Hide resolved
docat/docat/utils.py Outdated Show resolved Hide resolved
docat/docat/utils.py Outdated Show resolved Hide resolved
docat/tests/conftest.py Show resolved Hide resolved
docat/tests/test_setup.py Outdated Show resolved Hide resolved
docat/tests/test_search.py Show resolved Hide resolved
@randombenj
Copy link
Member

Added my review for the backend, the frontend won't be reviewed as it will be refactored by @reglim anyway.

reglim added a commit that referenced this pull request Nov 21, 2022
@reglim reglim force-pushed the Feat/13-Add-Search branch 2 times, most recently from fb87125 to 6ba0f62 Compare November 21, 2022 14:20
@reglim reglim force-pushed the Feat/13-Add-Search branch 4 times, most recently from 6bc6e80 to 6c59504 Compare November 22, 2022 10:59
Copy link
Member

@randombenj randombenj left a comment

Choose a reason for hiding this comment

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

The python part looks good to me, shall we merge and you rebase or shall we first merge the react migration?

@reglim
Copy link
Contributor Author

reglim commented Nov 23, 2022

We can merge, but first I think I'd remove the web part.

reglim added a commit that referenced this pull request Nov 30, 2022
@reglim reglim force-pushed the Feat/13-Add-Search branch from 6c59504 to dc209d3 Compare November 30, 2022 12:09
reglim added a commit that referenced this pull request Nov 30, 2022
We use an index database, which we expose via a search api. Tests are
also included in this commit. Some Models that were also used internally
are now renamed, and the folders used by Docat are created at startup.

fixes: #13, #320, #322
@reglim reglim force-pushed the Feat/13-Add-Search branch from dc209d3 to 472f44f Compare November 30, 2022 12:59
We use an index database, which we expose via a search api. Tests are
also included in this commit. Some Models that were also used internally
are now renamed, and the folders used by Docat are created at startup.

fixes: #13, #320, #322
@reglim reglim force-pushed the Feat/13-Add-Search branch from 472f44f to 827e469 Compare November 30, 2022 13:01
@reglim reglim requested a review from randombenj November 30, 2022 13:02
@reglim
Copy link
Contributor Author

reglim commented Nov 30, 2022

@randombenj It'd be nice if you could just glance over it just to make sure everything's fixed. I removed the web part and sqashed everything.

@reglim
Copy link
Contributor Author

reglim commented Nov 30, 2022

Also, when we release this, we need to communicate clearly, that the DOCAT_DOC_PATH is no longer used and replaced with DOCAT_STORAGE_PATH / doc. When you use the docker container, this is still the same, but locally it's not.
Also, we need to communicate that the database needs to be moved to DOCAT_STORAGE_PATH.

Copy link
Member

@randombenj randombenj left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the nice changes!
We can do the breaking changes announcement once we release.

@randombenj randombenj merged commit 4ca492f into main Dec 1, 2022
@fliiiix
Copy link
Member

fliiiix commented Dec 2, 2022

@reglim & @randombenj lets talk about the release timeline next week

also nice work @reglim

@reglim reglim mentioned this pull request Dec 6, 2022
@reglim reglim deleted the Feat/13-Add-Search branch January 4, 2023 10:17
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.

Implement search
3 participants