Salt: Proposal: Introduce a "line" state that replaces `file.line`

[JensRantil]

The file.line state (and its equivalent file.line module) is really hard to understand and use. While better documentation would help, I think both the module function and the state function simply wants to do too much. Therefor, it will be really hard to document and really hard for users to properly be able to use.

Proposal:

• Deprecate file.line state/module.
• Introduce a line state/module with more granular methods.
• This issue will be a collection of use-cases that the module and state should cover.

What do you think of this? If I get a couple of thumbs up I can get started adding use cases.

[cachedout]

I agree with you that the function is hard to understand, though I'm a little resistant to support a complete migration to a new state. My inclination is still break apart the functionality and simplify where we can, but keep the functionality in the state where it is already. Thoughts?

[JensRantil]

@cachedout I wish that would be possible, but I'm not sure it is. There are simply too many parameters and documenting file.line would require a fairly extensive description of all the different corner cases. There are too many combinations to actually be able to document all of them. I also think this boils down to have a razor sharp API, which will simplify code/testing and avoid bugs. Currently file.line doesn't have that and never will unless we reduce the number of input parameters.

[lorengordon]

I attempted to start such a discussion about the parameters to file.line in the PR where it was introduced, but it didn't really get very far. I'd be willing to participate here, if you want to toss out some ideas?

[lorengordon]

I think I agree it would improve the semantics to have line.py execution and state modules. Trying to cram all the functionality into one function is really messy.

With a separate execution module, we could make functions the action verb. i.e. line.replace, line.insert, line.delete, etc. Much of this can likely be wrappers around file.replace functionality.

Then the state module can have stateful function names, with more params controlling behavior in different conditions. i.e. line.present, line.absent, etc.

[damon-atkins]

A state should not always call an execution module. The current execution module open, read, writes and closes for every file request. It's a pity salt does not order states/actions for efficient instead of keep items in the order they are listed. For example make 10 line changes to a file that's 10 changes, not a consolidate single change to a file (assuming dependency does not prevent it).

[lorengordon]

@damon-atkins, salt _can_ do that. Are you familiar with the mod_aggregate capability? Just a matter of writing the function for the state module, and writing the execution module in a way that supports the desired functionality.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[lorengordon]

I think this is still relevant

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.

[sagetherage]

Assigning this to myself to get traction on a discussion

[waynew]

@sagetherage for some history (I actually wasn't aware of this issue), I've talked with Tom about it and file.line should absolutely be split into individual, well-tested functions. As it is, it's inconsistent and not recommended. Feel free to hit me up for more info 😂

[sagetherage]

@waynew can you please update this with any other labels, I have removed it from Blocked milestone and gave it Bug but likely it needs more and will need more details on next steps. ty