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

Recipes Improvements #16229

Open
MikeAlhayek opened this issue Jun 3, 2024 · 19 comments
Open

Recipes Improvements #16229

MikeAlhayek opened this issue Jun 3, 2024 · 19 comments
Milestone

Comments

@MikeAlhayek
Copy link
Member

One of the issues with recipes is that we currently tie a recipe to a specific feature when a module has multiple features. This should not longer be a limitation.

Since #15793 was merged, we should be able to tie a service to a feature with no problems. With that, we should be able to register multiple instanced of the RecipeDescriptor where each RecipeDescriptor will have info needed to locate the recipe file.

Then we probably, would remove IRecipeHarvester and it's implementation since this info will be in the IoC.

@MikeAlhayek MikeAlhayek added this to the 2.x milestone Jun 3, 2024
Copy link
Contributor

github-actions bot commented Jun 3, 2024

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

@Piedone
Copy link
Member

Piedone commented Jun 4, 2024

Recipes don't need a feature (like in a root web project), or a feature to be enabled to be available (since a recipe in a module may also enable features in that module, what many do).

So, I don't see tying a recipe to a feature as useful, but on the contrary, it would break important scenarios.

@MikeAlhayek
Copy link
Member Author

Recipes don't need a feature (like in a root web project), or a feature to be enabled to be available (since a recipe in a module may also enable features in that module, what many do).

That is valid for startup recipe but not for partial recipes that show up on the recipe admin page. But even for startup recipe, They should be tied to a feature. this way if you don't want some of the startup recipe to show up, then you won't make them available. One way this could be done is using a feature that is always enabled to enable the startup recipes only.

So, I don't see tying a recipe to a feature as useful, but on the contrary, it would break important scenarios.

We should be able to cover all scenarios even after assign them to feature. As mentioned above, one can create always-available feature to expose the all or subset of the startup recipes. In some case, you may want to limit the available startup recipes based on the environment. So in that case, you can have multiple features in the same module that would provide you with subsets of the startup recipes that should be available.

@Piedone
Copy link
Member

Piedone commented Jun 9, 2024

Taking a step back, what do we try to solve? What's in the current system, that works and I haven't seen people complaining about, that's unsuitable?

Not having all recipes listed on the setup screen is a solved problem.

@MikeAlhayek
Copy link
Member Author

I want to be able to control when recipes appear in the UI. Sometimes you want to tie a recipe to a feature in a module and not to the module itself as explain previously.

Also, for startup recipes, I want to be able to tie startup recipes to a feature as explain as well where I can use the same code with multiple environment where environment X can have a subset of startup recipes while environment B have another subset of startup recipes.

If others are not complaining, that does not mean there isn't an issue. Note that you had to create a feature to solve part of the problem because OC does not have a complete solution. Otherwise, why would you have that feature.

@Piedone
Copy link
Member

Piedone commented Jun 10, 2024

That's a fair point. How would we keep supporting the scenario of the recipe in a module with otherwise not yet enabled features being available, also enabling itself? This is a valid scenario for non-setup recipes as well, and something that recipes that initialize optional feature sets employ.

@MikeAlhayek
Copy link
Member Author

I would create an always enabled feature and I would place these kinds of recipes into. This way you have full control on which recipes make sense to enable all the time vs recipes that depend on other features. Remember that there are recipes that we want available only if an X feature is enabled too.

@Piedone
Copy link
Member

Piedone commented Jun 10, 2024

Do you mean to have an always-enabled feature for every module, then, that by default does nothing but you can add a corresponding Startup?

@MikeAlhayek
Copy link
Member Author

No. I mean in the case when you want a recipe to be always enabled, then you can create a feature and assign all the "always-enabled" recipes to it.

@Piedone
Copy link
Member

Piedone commented Jun 10, 2024

Hmm, and how that would be implemented? There's a chicken-and-egg problem, since even IsAlwaysEnabled = true features need to be enabled once. I'm not sure how I'd feel about adding something like an EnabledByDefault.

@agriffard agriffard changed the title Recipes Improvments Recipes Improvements Jun 10, 2024
@MikeAlhayek
Copy link
Member Author

I would create a new module for recipes (you can split that module into multiple features as needed). One or more of the features in the module will have IsAlwaysEnabled set to true. Now you can associate the recipes that you always want to make available to this feature. Same true for startup recipes too. The idea is just to associate them to a feature and then control the behavior of that feature as needed "when needed".

@Piedone
Copy link
Member

Piedone commented Jun 10, 2024

What would enable that module, though? Or are we talking about a special module that the container building/shell start logic knows about?

@MikeAlhayek
Copy link
Member Author

You can use AddGlobalFeatures() from the web app startup to ensure it's always enabled.

@Piedone
Copy link
Member

Piedone commented Jun 11, 2024

OK! Let's just not require anybody to have to explicitly enable this.

@hishamco
Copy link
Member

What's the summary for this enhancement :) seems I lost for the last 3 days because I got a new baby

@Piedone
Copy link
Member

Piedone commented Jun 15, 2024

Use case: #16229 (comment) Possible implementation: #16229 (comment)

Is that some kind of figure of speech or indeed you became a father? If the latter, then a hearty congratulations, Hisham!

@hishamco
Copy link
Member

Is that some kind of figure of speech or indeed you became a father? If the latter, then a hearty congratulations, Hisham!

Latter :) thanks so much, hope to join OC community ASAP

@Piedone
Copy link
Member

Piedone commented Jun 15, 2024

Oh, awesome, congratulations! :)

@hishamco
Copy link
Member

Thanks Zoltan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants