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

Refactored Computing API Components #28

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Refactored Computing API Components #28

wants to merge 24 commits into from

Conversation

TibbersHao
Copy link
Member

This PR addresses issue #27 to reflect the new decision of how content registry will interact with computing api.

Major Feature Modification/Upgrades:

  • Took our the box of job related panels.
  • Removed dependency of computing_api network in docker-compose file
  • In addition, content registry now will spawn its own mongo dB
  • Added gh actions
  • Added pre-commits for formatting
  • Added pyproject

Minor Upgrades:

  • Modified README as a reflection of changes

Note: the gh action and pre-commits still need testing. Right now it's in the very initial stage where files have been copied over and added.

@TibbersHao TibbersHao added bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request labels Mar 25, 2024
@TibbersHao TibbersHao requested a review from taxe10 March 25, 2024 23:12
@TibbersHao TibbersHao mentioned this pull request Mar 25, 2024
Open
@TibbersHao
Copy link
Member Author

@taxe10 reformatting has been finished successfully and all pre-commit checks have been passed. Ready to be reviewed and tested.

taxe10
taxe10 requested changes Mar 27, 2024
View reviewed changes
Copy link
Member

@taxe10 taxe10 left a comment

Choose a reason for hiding this comment

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

Great work Tibbers. I see that the frontend doesn't present the callback errors anymore. I left some minor comments that should be addressed before merging.

README.md Outdated Show resolved Hide resolved
content-api/docker/Dockerfile Outdated Show resolved Hide resolved
content-api/docker/Dockerfile Outdated Show resolved Hide resolved
content-api/src/content_api.py Outdated
USER_API_PORT = config["user api port"]["USER_API_PORT"]
SEARCH_API_PORT = config["search api port"]["SEARCH_API_PORT"]
MONGO_DB_USERNAME = str(os.environ["MONGO_INITDB_ROOT_USERNAME"])
MONGO_DB_PASSWORD = str(os.environ["MONGO_INITDB_ROOT_PASSWORD"])
Copy link
Member

Choose a reason for hiding this comment

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

I think we can also use os.getenv to retrieve the env variables as strings

Copy link
Member

Choose a reason for hiding this comment

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

This is failing at my end

content-regist/docker/Dockerfile Outdated Show resolved Hide resolved
content-regist/src/content_registry.py Outdated Show resolved Hide resolved
content-regist/src/content_registry.py Outdated Show resolved Hide resolved
content-regist/src/registry_util.py Outdated Show resolved Hide resolved
env.example Outdated Show resolved Hide resolved
data/dump/lbl-mlexchange/apps.metadata.json
{"indexes":[{"v":{"$numberInt":"2"},"key":{"_id":{"$numberInt":"1"}},"name":"_id_"}],"uuid":"e0ee1d18ee3140948f34a09fd32a96cf","collectionName":"apps","type":"collection"}
Copy link
Member

Choose a reason for hiding this comment

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

Did these files change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. These files have been tracked by Git and I haven't seen any changes so far during operation.

@TibbersHao
Copy link
Member Author

@taxe10 all comments have been addressed and new commits are ready to be reviewed!

taxe10
taxe10 requested changes Mar 28, 2024
View reviewed changes
Copy link
Member

@taxe10 taxe10 left a comment

Choose a reason for hiding this comment

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

Does the content api run at your end? I think the function os.getenv() should be followed by parenthesis instead of squared brackets.

@TibbersHao
Copy link
Member Author

@taxe10 The latest commit addressed an issue regarding duplicated mongo container name:

Issue

Previously the content registry is built on top of the computing api. With recent changes we are spawning a stand-along container named mongodb. If the mongodb container from computing api is pre-existed, this will run into error with duplicated container name, and the new mongodb container cannot be built. Vise versa, if content registry is built first, then computing api won't be built successfully due to same error.

Solution

Renamed the container name in docker-compose file to be content registry specific:
mongodb -> content-mongodb
And the mongo uri will need to be modified accordingly.

Side Thought

We may consider the similar modification for computing api too, the name mongodb might be a bit general, and we could run into similar issues in the future when developing mongo-related applications. I will open an issue in computing api repo along with some other suggestions in mind.

@TibbersHao
Copy link
Member Author

@taxe10 FYI the container name has been reverted back to "mongodb" based on this morning's discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taxe10 taxe10 taxe10 requested changes

Assignees
No one assigned
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TibbersHao @taxe10