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

keep module.run #58705

Closed
cmcmarrow opened this issue Oct 9, 2020 · 10 comments
Closed

keep module.run #58705

cmcmarrow opened this issue Oct 9, 2020 · 10 comments

Comments

@cmcmarrow
Copy link
Contributor

Salt had decided to keep module.run. This is because the replacement function is missing features and has a sharper learning curve.

@cmcmarrow cmcmarrow added the Feature new functionality including changes to functionality and code refactors, etc. label Oct 9, 2020
@cmcmarrow cmcmarrow added Deprecation and removed Feature new functionality including changes to functionality and code refactors, etc. labels Oct 9, 2020
@cmcmarrow cmcmarrow changed the title [FEATURE REQUEST] keep module.run keep module.run Oct 9, 2020
@OrangeDog
Copy link
Collaborator

What does this mean exactly? You mean the old syntax for a module.run state is no longer deprecated and will never be removed?
What effect does this have on e.g. #57919?

@cmcmarrow
Copy link
Contributor Author

cmcmarrow commented Oct 12, 2020

@OrangeDog right now we are planing for keeping the old syntax for module.run. There is no plan on removing the old syntax. I don’t believe this should effect #57919.

@OrangeDog
Copy link
Collaborator

Well it certainly removes the easy solution - remove both the deprecated syntax and the config option.

@sagetherage sagetherage added this to the Aluminium milestone Oct 12, 2020
@sagetherage sagetherage added the Aluminium Release Post Mg and Pre Si label Oct 12, 2020
@max-arnold
Copy link
Contributor

@cmcmarrow Could you please clarify the missing features part? What exactly is missing from the new module.run implementation?

@OrangeDog
Copy link
Collaborator

I'm also a bit confused, because the new syntax allows for more expression, not less, and should be easier to learn because it's closer to the general state syntax.

@sagetherage
Copy link
Contributor

I brought this up with the Core team and the new module.run is complex, but in use, it does make it easier to write states, but the learning curve is steep. The team wanted to properly deprecate the new module.run in a year since this would break states. I can't really say more as that is all I know, but I have asked @dwoz to chime in, here to answer your question @OrangeDog regarding syntax.

@dwoz
Copy link
Contributor

dwoz commented Oct 16, 2020

We, the core team, including myself had a conversation about this and we did come to a consensus to not deprecate the old way. The reasoning for not wanting to deprecate the old way is that people are extremely sensitive to changes which cause existing state trees to break. Breaking changes like this can prevent people from upgrading to newer versions of Salt and we want people to move forward.

Having said that, I also think we need to listen to the community. I would like to think there are ways we can move forward and make significant improvements when it makes sense. After digging into this change a bit more, I am on the fence on this one. On one hand the syntax introduced by this change is clearly cleaner and more intuitive. I do not think anyone will argue that point. On the other hand, the change is mainly just syntactical. The only significant feature that I see added by the new syntax is the ability to pass *args to the function being called. I should be clear that I am not a fan of module functions accepting *args and/or **kwargs. My opinion is that does not warrant a breaking change. So for me, the question comes down to whether or not having cleaner and more intuitive syntax warrants the breaking change.

@max-arnold
Copy link
Contributor

What about gradually phasing out the old syntax?

@OrangeDog
Copy link
Collaborator

OrangeDog commented Oct 16, 2020

I'd argue that going through a full deprecation cycle relying on a misnamed config option, delaying it (it used to say it would be removed in Sodium), and then reversing it is a lot more disruptive. You can't go back to just the old version, because then the problems for which it was replaced come back again. If you really want to keep it for some reason, then just change the code to handle both syntaxes without needing a config change, or at minimum make the new syntax the default.

This is also confusing as the previous messaging was you were expecting to making breaking changes every major release. So why is this already almost already complete change suddenly a problem?

@sagetherage sagetherage modified the milestones: Aluminium, Silicon Mar 24, 2021
@sagetherage sagetherage added the Silicon v3004.0 Release code name label Mar 24, 2021
@sagetherage sagetherage assigned cmcmarrow and unassigned Ch3LL May 19, 2021
@sagetherage sagetherage removed the Silicon v3004.0 Release code name label Aug 12, 2021
@sagetherage sagetherage removed this from the Silicon milestone Aug 12, 2021
@OrangeDog
Copy link
Collaborator

Resolved by #61772

@OrangeDog OrangeDog added this to the Phosphorus v3005.0 milestone Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants