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

Storing in activity table #64

Merged
merged 9 commits into from
Nov 11, 2022
Merged

Storing in activity table #64

merged 9 commits into from
Nov 11, 2022

Conversation

renan-souza
Copy link
Collaborator

#61

- Enabling autoupdate in the installed lib while developing through `pip install -e .` in the Dockerfile
- Getting the dependencies from the requirements.txt in setup.py
- Using the env var from config.py for the ZMQ port in settings.py
- Improving the compose.yml to get logs and wait.
#61

- Enabling saving and updating actitivites in SQLite
- Adding the schema (DDL)
- Updating requirements.txt to use SQLAlchemy
- Creating the code structure for a Data Access Object (DAO) pattern
- Adding volumes to the docker compose to persist the saved data in the database
- Each agent creates a local database (if there's any yet) in settings.py and stores the activity data in the agent.py
- Adding unit tests for these features
@renan-souza renan-souza changed the title Renan activity table Storing in activity table Nov 2, 2022
Copy link
Collaborator

@JoshuaSBrown JoshuaSBrown left a comment

Choose a reason for hiding this comment

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

I only have one issue that I think must be resolved before this is merged, a different name then Activity should be used, because that is already defined in the campaign folder. Everything else is optional.

.gitignore Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
setup.py Show resolved Hide resolved
deployment/compose.yml Show resolved Hide resolved
zambeze/orchestration/db/dao/activity_dao.py Show resolved Hide resolved
zambeze/orchestration/db/dao/__init__.py Outdated Show resolved Hide resolved
zambeze/orchestration/db/model/abstract_entity.py Outdated Show resolved Hide resolved
zambeze/orchestration/db/model/activity.py Outdated Show resolved Hide resolved
deployment/Dockerfile Show resolved Hide resolved
zambeze/settings.py Show resolved Hide resolved
Copy link
Collaborator Author

@renan-souza renan-souza left a comment

Choose a reason for hiding this comment

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

This commit addresses the feedback received by Joshua and Rafael. 8eb2c82

zambeze/orchestration/db/dao/__init__.py Outdated Show resolved Hide resolved
zambeze/orchestration/db/model/abstract_entity.py Outdated Show resolved Hide resolved
zambeze/orchestration/db/model/activity.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@renan-souza renan-souza left a comment

Choose a reason for hiding this comment

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

This commit addresses Joshua's and Rafael's feedback 8eb2c82

Comment on lines +16 to +21

@classmethod
@property
@abstractmethod
def ENTITY_NAME(cls) -> str:
raise NotImplementedError()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this what you meant about constants? This is essentially a constant member of the class right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add something in the CONTRIBUTING.md page about what is expected for constants.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What does passing in (cls) as the parameter do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What does passing in (cls) as the parameter do?

This is the "self" argument but for a class-level method. Since this is supposed to be a class-level (as opposed to instance-level) constant, I believe cls is right.

Copy link
Collaborator

@JoshuaSBrown JoshuaSBrown left a comment

Choose a reason for hiding this comment

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

Great pr, positive change. I have a few questions to learn from what you are doing but I approve.

@renan-souza renan-souza merged commit 974cd88 into devel Nov 11, 2022
@renan-souza renan-souza deleted the renan-activity-table branch January 24, 2023 18:32
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.

3 participants