-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
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.
Thanks @misohu, I have left some comments mainly focusing on the UX.
This PR needs to be postponed until this is resolved #58 |
The dss remove command has been discussed with UX team please check the DSS spec here for details about the error messages and parameters. |
240de2e
to
af44012
Compare
All the comments from UX team are now part of this PR |
af44012
to
c495346
Compare
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.
Left some comments and looking into tests 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.
Great test coverage actually, congratz 🎉
c495346
to
8bbd2b5
Compare
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.
Thanks Michal, great work!
Please first review this one #62
This PR resolves issue: #33
dss logs
with--mlflow
and--all
flagsHow to test:
If you want to test locally run the steps from .github/workflows/tests.yaml:integration (integration tests)