-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Base version - Finalize #5
Conversation
…into base-version
Resolved most vital issues for some sort of base version (no operational but it has all the infrastructurefor development now). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code is very good, and some of the comments I made relate to coding style.
I am sure your code has some bugs from this CR and it is apparent that you didn't run it with "devices". it is missing noticably.
Notice I only reviews the backend, I can also go over the frontend but it is perhaps better if someone better than me in frontend and react reviews it
.env
Outdated
DB_PORT=27017 | ||
DB_NAME=sentinel | ||
|
||
DB_ROOT_USER=root123 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't really have a better option for now, but in Kubernetes it is more secure to add username and password details using secrets, and if you use terraform with it and a nice secrets manager in the cloud, it doesn't even have to be written anywhere unsecure or in your repo at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for the informative comment
DB_SERVER: ${{ secrets.DB_SERVER }} | ||
DB_PORT: ${{ secrets.DB_PORT }} | ||
DB_USER: ${{ secrets.DB_USER }} | ||
DB_PASSWORD: ${{ secrets.DB_PASSWORD }} | ||
DB_NAME: ${{ secrets.DB_NAME }} | ||
PAT: ${{ secrets.PAT }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replications where you put all these details in both the .env file and the repo secrets is bad, try and find a way to put it in one place or not at all
because it is for tests, it is even good if you create random user and password with fictive details for the test...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i will transition to docker-compose based action this solution to run mongoDB is lacking... i will update once its done
|
||
# Install dependencies | ||
|
||
- name: Install Poetry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to create an image with poetry already installed if you can.
Notice that if you run tests on every push, every second of the test running is crucial
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there are some Poetry-ready images available, but since this is the foundational version, this solution is sufficient for now.
README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, please resolve
ARG INSTALL_DEV=false | ||
RUN if [ "$INSTALL_DEV" = "true" ]; then poetry install --no-root; else poetry install --no-root --only main; fi | ||
|
||
# Stage 1, Final stage, to have only the compiled dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure it is worth to split to multi stage dockerfile, only for cleaning the build files of poetry.
It is good to know but multi stage dockerfile is for if you build 2 indpenedent stuff that run together in the end (for build concurrency)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this multi-staging aims to shrink the docker image size by a little, i like it although the convention is to have a poetry image and just install on it the project dependencies.
although it will make the docker cleaner i find this code cool and the change will lead for almost no noticable benefits in the current project scope...
backend/src/api/routers/device.py
Outdated
from src.dal.entities.device import Device | ||
|
||
router = APIRouter(prefix="/device", | ||
tags=["device"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why newline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vscode formatter, fixed in the next commit
Literal[Environment.DEVELOPMENT, Environment.PRODUCTION], | ||
BeforeValidator(parse_environment) | ||
] = Environment.DEVELOPMENT | ||
WORKER_COUNT: int = os.cpu_count() * 2 + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, resolve please
|
||
@computed_field | ||
@property | ||
def MONGO_DATABASE_URI(self) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, resolve please
backend/src/core/config/app.py
Outdated
PORT: int = 5000 | ||
CORS_ORIGINS: Annotated[list[AnyUrl] | str, BeforeValidator(parse_cors)] = [] | ||
ENVIRONMENT: Annotated[ | ||
Literal[Environment.DEVELOPMENT, Environment.PRODUCTION], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the literal useless when the validator is present, even for typing for code instance of the settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess so changed it..
return Device(**device_data) | ||
|
||
|
||
class DeviceRepository: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the repository also cache stuff about the devices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a foundational version of the project. While I anticipate that caching will be necessary in the future, this solution is sufficient for now.
This pull request addresses the renaming and refinement of the base version of this project.
The original branch, named
base_abstract_version
, has been renamed to better reflect the current state of the project.The base version has evolved significantly and is no longer abstract, requiring mostly frontend work before it can be merged into the main branch.
My current TODO list:
Implementing Backend Tests
Multi Stage Docker - Frontend (Implementing Ngnix)
Fixing Docker Compose - Backend connection with the MongoDB
Implementing Fully TypeScript (currently its partial)
After base version:
it may change according to the flow and direction this project will take over the basic version..