Skip to content

PR: Updated README for local Elastic Search, And Refactor Elastic Search Config#82

Merged
kouloumos merged 5 commits intobitcoinsearch:masterfrom
elraphty:feat/local_setup
Oct 15, 2024
Merged

PR: Updated README for local Elastic Search, And Refactor Elastic Search Config#82
kouloumos merged 5 commits intobitcoinsearch:masterfrom
elraphty:feat/local_setup

Conversation

@elraphty
Copy link
Copy Markdown
Contributor

@elraphty elraphty commented Oct 3, 2024

This PR:

  • Refactored the elastic search connection to be able connect to local and production
  • It also updated the README file
  • And added a config package to be used by other scrapper packages.

@elraphty elraphty changed the title PR: Updated README for local Elastic Search PR: Updated README for local Elastic Search, And Refactor Elastic Search Config Oct 3, 2024
Copy link
Copy Markdown
Member

@kouloumos kouloumos left a comment

Choose a reason for hiding this comment

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

looks good, I left a small nit for the import of all env variables from config

Comment thread bitcoin.stackexchange.com/main.py Outdated
Comment on lines +13 to +16
from config.conf import INDEX_NAME, DATA_DIR

BASE_DIR = os.getenv("DATA_DIR", ".")
if __name__ == "__main__":
BASE_DIR = os.getenv(DATA_DIR, ".")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you can also import BASE_DIR from config.conf.

Additionally, if you wanted to do the config a bit more flexible you could structure it like this one here, but I don't consider it a must for now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

you can also import BASE_DIR from config.conf.

Additionally, if you wanted to do the config a bit more flexible you could structure it like this one here, but I don't consider it a must for now.

Okay, thanks @kouloumos

@elraphty
Copy link
Copy Markdown
Contributor Author

elraphty commented Oct 4, 2024

looks good, I left a small nit for the import of all env variables from config

Thanks @kouloumos I have moved BASE_DIR to the config/conf.py

@kouloumos
Copy link
Copy Markdown
Member

[...] I have moved BASE_DIR to the config/conf.py

It seems that BASE_DIR == DATA_DIR, so we don't need both variables, right?

As I said, this is a nit, but we already started discussing it, so let's fix it and merge.

@elraphty
Copy link
Copy Markdown
Contributor Author

elraphty commented Oct 8, 2024

[...] I have moved BASE_DIR to the config/conf.py

It seems that BASE_DIR == DATA_DIR, so we don't need both variables, right?

As I said, this is a nit, but we already started discussing it, so let's fix it and merge.

Oh!, I thought they served different purposes, making an update now.

Comment thread mailing-list/package.json Outdated
Copy link
Copy Markdown
Member

@kouloumos kouloumos Oct 9, 2024

Choose a reason for hiding this comment

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

I think that main.js contains code that we are not using anymore, as we transitioned to main.py, so maybe we can delete both package.json, yarn.lock and main.js as part of this PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

Copy link
Copy Markdown
Member

@kouloumos kouloumos left a comment

Choose a reason for hiding this comment

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

Looks good!

@kouloumos kouloumos merged commit ad894da into bitcoinsearch:master Oct 15, 2024
kouloumos pushed a commit to kouloumos/scraper that referenced this pull request Nov 13, 2024
- Refactored the elastic search connection to be able connect to local and production
- It also updated the README file
- And added a config package to be used by other scrapper packages.
- deleted unused javascript files from previous mailing-list implementation
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.

2 participants