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

Remove the need for static MinionConfiguration #13

Open
matt-psaltis opened this issue Mar 14, 2019 · 2 comments
Open

Remove the need for static MinionConfiguration #13

matt-psaltis opened this issue Mar 14, 2019 · 2 comments

Comments

@matt-psaltis
Copy link

We often deploy logically separate services as a single deployment unit using a single service or web app as the host and then boot up the logically separate services within the same application / app domain.

Due to the static Configuration property of the MinionConfiguration, this can cause issues when the MinionConfiguration is configured by multiple logical services. I've had a cursory look and it appears that the static nature of the MinionConfiguration can be fairly easily removed. This would mean that a MinionConfiguration instance is constructed and passed into BatchEngine for example as a constructor parameter.

var minionConfiguration = new MinionConfiguration();
minionConfiguration.UseSqlStorage("<connection string>");

var engine = new BatchEngine(minionConfiguration);
engine.Start();

Is this something you'd be open to adding?

@pug-pelle-p
Copy link
Contributor

Hi! As of right now, you can use the BatchEngine without the static configuration, just use the other constructor to inject all needed dependencies.

I'm thinking of using the generic host (https://docs.microsoft.com/en-us/aspnet/core/fundamentals/host/generic-host?view=aspnetcore-2.2) as the primary hosting model for Minion. Which I hope will allow for a cleaner way to configure the BatchEngine (Thru DI), but I haven't looked into this yet.

I'm open to suggestions on how to make this a better experience :)

@matt-psaltis
Copy link
Author

matt-psaltis commented Mar 14, 2019

No worries, my concern with using the second constructor is it bypasses the argument validation rules particularly in batch engine. Additionally the configuration extension methods don't pass the MinionConfiguration instance represented by "this" in the extension method to the underlying object. This has the effect of the wrong date service being applied in the case of say the SQL Storage object. It just reduces the syntax sugar usefulness a bit.

We're having a lot of success with generic host builder. It might be worth looking at what's coming in net core 3.0 with the consolidation of generic host and Web host builder.

Thanks for feedback

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

No branches or pull requests

2 participants