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

feat: added renku service with cache and datasets #788

Merged
merged 19 commits into from
Dec 17, 2019
Merged

feat: added renku service with cache and datasets #788

merged 19 commits into from
Dec 17, 2019

Conversation

jsam
Copy link
Contributor

@jsam jsam commented Nov 1, 2019 •
edited
Loading

addresses: #732

closes: #770

@jsam jsam requested a review from a team as a code owner November 1, 2019 07:42
@jsam jsam force-pushed the 732_datasets_svc branch 9 times, most recently from 98b6e70 to 3c2bc8d Compare November 7, 2019 16:56
Panaetius
Panaetius reviewed Nov 8, 2019
View reviewed changes
renku/service/entrypoint.py Outdated Show resolved Hide resolved
renku/service/entrypoint.py Show resolved Hide resolved
renku/service/views/__init__.py Outdated Show resolved Hide resolved
renku/service/views/__init__.py Outdated Show resolved Hide resolved
rokroskar
rokroskar reviewed Nov 10, 2019
View reviewed changes
renku/service/views/__init__.py Outdated Show resolved Hide resolved
renku/core/management/datasets.py Outdated Show resolved Hide resolved
rokroskar
rokroskar reviewed Nov 10, 2019
View reviewed changes
renku/service/Dockerfile Outdated Show resolved Hide resolved
rokroskar
rokroskar reviewed Nov 10, 2019
View reviewed changes
renku/service/config.py Outdated Show resolved Hide resolved
rokroskar
rokroskar requested changes Nov 10, 2019
View reviewed changes
Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

Nice to see this taking shape!

I think it would be nice to be more consistent about the use of client - I don't see much reason for hacking around it and allowing it to be created implicitly. IMO would be much better (and clearer) to be explicit about this.

Would also be great to have a high-level overview of the design and the API in the PR and/or in the issues that it refers to. Ideally, sequence diagrams in the docs would accompany this work.

renku/service/views/cache.py Outdated Show resolved Hide resolved
renku/service/views/cache.py Outdated Show resolved Hide resolved
renku/service/views/cache.py Outdated Show resolved Hide resolved
renku/service/views/datasets.py Outdated Show resolved Hide resolved
renku/core/commands/client.py Outdated Show resolved Hide resolved
renku/service/views/datasets.py Outdated Show resolved Hide resolved
renku/service/views/datasets.py Outdated Show resolved Hide resolved
renku/service/views/datasets.py Outdated Show resolved Hide resolved
renku/service/views/__init__.py Outdated Show resolved Hide resolved
renku/service/views/__init__.py Outdated Show resolved Hide resolved
jsam
jsam commented Nov 13, 2019
View reviewed changes
Copy link
Contributor Author

@jsam jsam left a comment

Choose a reason for hiding this comment

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

Thanks for the review. Let's discuss the points on suggested changes. Many of them to make very little sense to me, so maybe I'm missing something from the suggestions.

renku/service/views/datasets.py Show resolved Hide resolved
renku/service/views/datasets.py Outdated Show resolved Hide resolved
@jsam jsam force-pushed the 732_datasets_svc branch 5 times, most recently from 92220ba to 2e33e6f Compare November 22, 2019 20:40
@jsam jsam force-pushed the 732_datasets_svc branch from d339e66 to 4a1ac4a Compare December 3, 2019 07:24
@jsam jsam changed the title [WIP] feat: added renku service with cache and datasets feat: added renku service with cache and datasets Dec 3, 2019
@jsam jsam requested a review from rokroskar December 3, 2019 07:52
@jsam jsam force-pushed the 732_datasets_svc branch from 67b4736 to 5d0859e Compare December 3, 2019 14:46
@rokroskar rokroskar requested a review from m-alisafaee December 4, 2019 15:24
Panaetius
Panaetius requested changes Dec 11, 2019
View reviewed changes
Copy link
Member

@Panaetius Panaetius left a comment

Choose a reason for hiding this comment

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

Looks really great with the new redis backend, much better than before! Thank you 👍

Overall i think this is ready to merge, I just found two minor issues.

renku/core/management/clone.py Outdated Show resolved Hide resolved
renku/service/views/cache.py Show resolved Hide resolved
@jsam jsam requested a review from Panaetius December 17, 2019 13:54
Panaetius
Panaetius approved these changes Dec 17, 2019
View reviewed changes
Copy link
Member

@Panaetius Panaetius left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@jsam jsam merged commit 7a7068d into master Dec 17, 2019
@rokroskar rokroskar deleted the 732_datasets_svc branch December 18, 2019 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rokroskar rokroskar rokroskar approved these changes

@Panaetius Panaetius Panaetius approved these changes

@m-alisafaee m-alisafaee Awaiting requested review from m-alisafaee

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RaaS: Add uploaded file to project
3 participants
@jsam @rokroskar @Panaetius