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

♻️ Is638/webserver's project and directorv2 plugins refactoring #3189

Merged
merged 16 commits into from
Jul 18, 2022
Merged

♻️ Is638/webserver's project and directorv2 plugins refactoring #3189

merged 16 commits into from
Jul 18, 2022

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Jul 13, 2022 •
edited

What do these changes do?

This refactoring is a pre-requisite to extend the web-server API to selectively start/stop/restart dynamic services (in PR ##3186):

  • ♻️ splits interface director_v2_core*.py plugin that wraps interaction with director-v2 service rest API:
    • director_v2_core_computations provides python APIs for the computations and cluster resources
      • NOTE: after grouping, it reveals duplication in the interface
    • director_v2_core_dynamic_services provides python APIs for the dynamic_services
    • director_v2_core_base/utils for common and utils
  • ♻️ minor doc and cleanup in projects plugin
    • doc/groups functions in projects/projects_api.py
    • moves events handlers into projects/projects_events.py
  • ♻️ cleanup webserver's OAS

Related issue/s

How to test

CI tests in place

@pcrespov pcrespov self-assigned this Jul 13, 2022
@pcrespov pcrespov added a:webserver issue related to the webserver service a:api framework api, data schemas, labels Jul 13, 2022
@codecov
Copy link

codecov bot commented Jul 13, 2022 •
edited

Codecov Report

Merging #3189 (85f338a) into master (85f338a) will not change coverage.
The diff coverage is n/a.

❗ Current head 85f338a differs from pull request most recent head 12b9762. Consider uploading reports for the commit 12b9762 to get more accurate results

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3189   +/-   ##
======================================
  Coverage    78.0%   78.0%           
======================================
  Files         701     701           
  Lines       29862   29862           
  Branches     3884    3884           
======================================
  Hits        23297   23297           
  Misses       5827    5827           
  Partials      738     738
Flag Coverage Δ
unittests 77.9% <0.0%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@pcrespov pcrespov changed the title WIP: ♻️ Is638/web project n dv2 plugins refactoring ♻️ Is638/webserver's project and directorv2 plugins refactoring Jul 14, 2022
@pcrespov pcrespov marked this pull request as ready for review July 14, 2022 02:32
@pcrespov pcrespov added this to the Meteora milestone Jul 14, 2022
@pcrespov pcrespov mentioned this pull request Jul 14, 2022
Closed
GitHK
GitHK reviewed Jul 14, 2022
View reviewed changes
Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

Please see if my comments below make sense.

services/web/server/src/simcore_service_webserver/director_v2_api.py
Comment on lines +28 to +38
from .director_v2_core_dynamic_services import (
DirectorServiceError,
get_dynamic_service_state,
get_dynamic_services,
request_retrieve_dyn_service,
restart,
restart_dynamic_service,
retrieve,
start_service,
stop_service,
stop_services,
update_cluster,
run_dynamic_service,
stop_dynamic_service,
stop_dynamic_services_in_project,
update_dynamic_service_networks_in_project,
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I don't like how it was previously done, and not how it is done right now. It does not make it clear.

Could I suggest we only export something like.

from . import director_v2_core_computations as api_comp
from . import director_v2_core_dynamic_services as api_dynamic

And where we need to use them we do it like the following:

from director_v2_api import api_dynamic

# example usage
api_dynamic.restart()

What do you think about this suggestion?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with the general idea of separating the apis. This is actually a step in that direction.

But it needs a bit more refactoring of the underlying code and tst structure before I would do the full separation. For instance, this plugin should not be centered in the director-v2,

or another example, the mock.patches should be easily modified: currently a simple change like the one you are suggesting above, it is a huge burden to make the tests pass.

services/web/server/src/simcore_service_webserver/director_v2_core_dynamic_services.py Outdated Show resolved Hide resolved
services/web/server/src/simcore_service_webserver/director_v2_core_dynamic_services.py Outdated Show resolved Hide resolved
mrnicegyu11
mrnicegyu11 approved these changes Jul 14, 2022
View reviewed changes
Copy link
Member

@mrnicegyu11 mrnicegyu11 left a comment

Choose a reason for hiding this comment

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

Thank you so much for this contribution, excited to see this progress :) Very much appreciated

services/web/server/src/simcore_service_webserver/director_v2_api.py Show resolved Hide resolved
services/web/server/src/simcore_service_webserver/director_v2_core_base.py Show resolved Hide resolved
services/web/server/src/simcore_service_webserver/director_v2_core_dynamic_services.py Show resolved Hide resolved
services/web/server/src/simcore_service_webserver/director_v2_core_dynamic_services.py Show resolved Hide resolved
services/web/server/src/simcore_service_webserver/director_v2_core_dynamic_services.py Outdated Show resolved Hide resolved
services/web/server/src/simcore_service_webserver/director_v2_core_utils.py Show resolved Hide resolved
@pcrespov pcrespov requested a review from GitHK July 14, 2022 08:31
@pcrespov pcrespov mentioned this pull request Jul 14, 2022
Open
pcrespov
pcrespov commented Jul 14, 2022
View reviewed changes
Copy link
Member Author

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

done

GitHK
GitHK approved these changes Jul 18, 2022
View reviewed changes
Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

👍 let's go for this, it will definitely work for now

@pcrespov pcrespov force-pushed the is638/web-project-n-dv2-plugins-refactoring branch from 5efc355 to 12b9762 Compare July 18, 2022 19:13
@sonarcloud
Copy link

sonarcloud bot commented Jul 18, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@pcrespov pcrespov merged commit 3185694 into ITISFoundation:master Jul 18, 2022
@pcrespov pcrespov deleted the is638/web-project-n-dv2-plugins-refactoring branch July 18, 2022 19:51
@GitHK GitHK mentioned this pull request Jul 28, 2022
Open
@sanderegg sanderegg mentioned this pull request Jul 28, 2022
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GitHK GitHK GitHK approved these changes

@mrnicegyu11 mrnicegyu11 mrnicegyu11 approved these changes

@sanderegg sanderegg Awaiting requested review from sanderegg sanderegg is a code owner

@odeimaiz odeimaiz Awaiting requested review from odeimaiz

@colinRawlings colinRawlings Awaiting requested review from colinRawlings

Assignees

@pcrespov pcrespov

Labels
a:api framework api, data schemas, a:webserver issue related to the webserver service
Projects
None yet
Milestone
Meteora
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pcrespov @GitHK @mrnicegyu11