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

Implement dss logs functionality. #55

Merged
merged 6 commits into from
Apr 5, 2024
Merged

Conversation

misohu
Copy link
Member

@misohu misohu commented Mar 21, 2024

Please first review this one #62

This PR resolves issue: #33

  1. Implemented dss logs with --mlflow and --all flags
  2. Implemented unit test for happy and bad scenarios
  3. Implemented integration test

How to test:
If you want to test locally run the steps from .github/workflows/tests.yaml:integration (integration tests)

@misohu misohu requested a review from a team as a code owner March 21, 2024 14:27
setup.cfg Outdated Show resolved Hide resolved
Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

Thanks @misohu, I have left some comments mainly focusing on the UX.

src/dss/logs.py Outdated Show resolved Hide resolved
src/dss/logs.py Show resolved Hide resolved
src/dss/main.py Outdated Show resolved Hide resolved
@misohu
Copy link
Member Author

misohu commented Mar 25, 2024

This PR needs to be postponed until this is resolved #58

@misohu
Copy link
Member Author

misohu commented Mar 27, 2024

The dss remove command has been discussed with UX team please check the DSS spec here for details about the error messages and parameters.

@misohu misohu force-pushed the KF-5382-implement-logs-command branch from 240de2e to af44012 Compare April 2, 2024 10:40
@misohu
Copy link
Member Author

misohu commented Apr 2, 2024

All the comments from UX team are now part of this PR

@misohu misohu force-pushed the KF-5382-implement-logs-command branch from af44012 to c495346 Compare April 4, 2024 08:50
Copy link
Contributor

@orfeas-k orfeas-k left a comment

Choose a reason for hiding this comment

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

Left some comments and looking into tests now!

setup.cfg Outdated Show resolved Hide resolved
src/dss/utils.py Outdated Show resolved Hide resolved
src/dss/logs.py Outdated Show resolved Hide resolved
src/dss/logs.py Show resolved Hide resolved
src/dss/logs.py Outdated Show resolved Hide resolved
src/dss/logs.py Outdated Show resolved Hide resolved
src/dss/logs.py Show resolved Hide resolved
src/dss/main.py Outdated Show resolved Hide resolved
src/dss/main.py Outdated Show resolved Hide resolved
src/dss/main.py Outdated Show resolved Hide resolved
Copy link
Contributor

@orfeas-k orfeas-k left a comment

Choose a reason for hiding this comment

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

Great test coverage actually, congratz 🎉

tests/integration/deploy.yaml Outdated Show resolved Hide resolved
tests/integration/test_dss.py Show resolved Hide resolved
@misohu misohu force-pushed the KF-5382-implement-logs-command branch from c495346 to 8bbd2b5 Compare April 4, 2024 13:36
Copy link
Contributor

@orfeas-k orfeas-k left a comment

Choose a reason for hiding this comment

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

Thanks Michal, great work!

@misohu misohu merged commit 517ffe6 into main Apr 5, 2024
3 checks passed
@misohu misohu deleted the KF-5382-implement-logs-command branch April 5, 2024 07:18
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