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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Add the ability to ignore services #1223

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mohsen-karami
Copy link

There are situations in that the user may want to ignore some services; this feature allows them to do this by simply adding them as an array and putting it as the third parameter of the loadServices method.

馃摑 Description

In our example, one service needed to install the TensorFlow package, which was incompatible with the hardware of some developers. Since they don鈥檛 code on the service and it was unnecessary for running the app, I鈥檝e added this feature, thus they can simply ignore the service. In our case, since we still want the service to be available for others, I defined an environment variable in the .env file and then wrote an if statement to ignore the intended services only if their relevant environment variable (in our case, EXCLUDED_SERVICES) exists.

馃拵 Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

馃弫 Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

There are situations in that the user may temporarily want to ignore some services; this feature allows them to do this by simply adding them as an array and putting it as the third parameter of the loadServices method.
@AndreMaz
Copy link
Member

AndreMaz commented Jul 3, 2023

Hi @mohsen-karami

You can ignore specific services via env vars. like:

SERVICES=src/services,!**/api.service.js // will load everything in src/services expect for the "api.service"

This approach is not enough for your use case?

@mohsen-karami
Copy link
Author

Hey @AndreMaz

Yeah, thanx.
Your approach works great if we run the project using the moleculer-runner, but since we鈥檙e running multiple instances of Moleculer and want to have even more control over them, we had to create the ServiceBroker manually.

I think this implementation is convenient for any other who runs the broker manually (for any possible reason!).

@icebob
Copy link
Member

icebob commented Jul 11, 2023

Thank you for your job, but as Andr茅 said, this functionality is covered in Runner with SERVICE env var, and if we implement the same logic into the broker as well, in the future it will confuse us and make it hard to maintain all implementation.

So at this point, I won't merge it because you can do it inside your project, if you override the loadServices method or add a new custom method in ServiceBroker with you logic.

By the way, if we faced that many users need this feature, we will consider it.

@mohsen-karami
Copy link
Author

mohsen-karami commented Jul 12, 2023

OK, thanx @icebob for the review.

you鈥檙e right, if it鈥檚 only us needing this feature, considering that it may add complexity and confusion to the project, and making maintenance more difficult for you, then I guess better to follow your approach.

@icebob
Copy link
Member

icebob commented Jul 12, 2023

@mohsen-karami Thank you for your understanding

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.

None yet

3 participants