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

Capabilities RFC #50617

Merged
merged 2 commits into from Dec 12, 2018
Merged

Capabilities RFC #50617

merged 2 commits into from Dec 12, 2018

Conversation

isbm
Copy link
Collaborator

@isbm isbm commented Nov 22, 2018

What does this PR do?

Describes presented on Salt Conference 2018 capabilities.

Please read it here.

@isbm isbm requested a review from as a code owner Nov 22, 2018
@ghost ghost self-requested a review Nov 22, 2018
@isbm
Copy link
Collaborator Author

@isbm isbm commented Nov 22, 2018

@cachedout bumpitybump!

dwoz
dwoz approved these changes Nov 23, 2018
Copy link
Contributor

@dwoz dwoz left a comment

I like this. Long term maybe we can combine it with some parameter spec decorators that could handle input validation and normalization. Assuming others agree, we'll probably want that as a separate RFC.
I approve of this one as it stands now.

@isbm
Copy link
Collaborator Author

@isbm isbm commented Nov 23, 2018

@dwoz that is certainly another RFC. The input validation/normalisation is another topic and mostly relevant to the Interface RFC. On which you are also welcome to comment. 😉

@cachedout cachedout self-requested a review Nov 26, 2018
Copy link
Contributor

@cachedout cachedout left a comment

I think this is a very smart way to do this. One thought might be that we could perhaps simplify this even further by creating a custom jinja filter?

@thatch45
Copy link
Member

@thatch45 thatch45 commented Nov 26, 2018

As much as I don't like having deeply nested objects like this, I do think that this is a good overall design. If there is a way to make this not as nested I would like it more.

@dwoz dwoz added the ZRETIRED - RFC label Nov 26, 2018
@isbm
Copy link
Collaborator Author

@isbm isbm commented Nov 26, 2018

@cachedout it already is a Jinja filter. We only might add "shortcuts", but personally I afraid to clash with something else. Namespace is very good.

@thatch45 well, if not nested, then we will have it like PHP. We then will be risking having clashes all over the place.

@cachedout
Copy link
Contributor

@cachedout cachedout commented Nov 26, 2018

@isbm Perhaps we could explore a way to "reserve" shortcuts somewhere in Salt to avoid clashes. It would be good if we could make these as short as possible while still avoiding potential clashes.

@isbm
Copy link
Collaborator Author

@isbm isbm commented Nov 26, 2018

But how? You need to start somewhere. Either it is a module or state. Then you have to refer to a function of that module. Then you should select what area you want to explore and so on. You cannot just say "reboot me a machine if 'foo' in documentation". Because Salt has absolutely no idea what exactly is that you want from it.

Having shortcuts will make it harder because one will need to remember many things instead of one.

Having a single entry also allows us to extend it to other areas I already wrote in the RFC as well as even different places yet to come. We are not going to pollute namespace with the whole sack of new reserved words, being afraid to screw up someone's SLS. 😀

Also single entry proposes to do this step by step, smoothly and invisible for whole current ecosystem of Salt users. Especially that we already have "salt" reserved. So we can simply extend that, instead of "capable":

salt.modules.network.ping.doc.search....
salt.netwotk.ping(hostname)
salt['network.ping'](hostname)

In first case we introspect. In second we call. Third is just current way, doing the same as the second example.

@whytewolf
Copy link
Contributor

@whytewolf whytewolf commented Nov 27, 2018

I love the idea of this. But I did want to note. That the "bonus feature" of salt.module.function already exists in salt. https://docs.saltstack.com/en/latest/topics/jinja/index.html#calling-salt-functions

https://github.com/saltstack/salt/blob/develop/salt/renderers/jinja.py#L23-L42

However, if dropped in favor of this I would not complain.

Also if this augmented the sysmod functionality that would be a good bonus.
https://docs.saltstack.com/en/latest/ref/modules/all/salt.modules.sysmod.html

@isbm
Copy link
Collaborator Author

@isbm isbm commented Nov 27, 2018

I love the idea of this. But I did want to note. That the "bonus feature" of salt.module.function already exists in salt. https://docs.saltstack.com/en/latest/topics/jinja/index.html#calling-salt-functions

https://github.com/saltstack/salt/blob/develop/salt/renderers/jinja.py#L23-L42

Syntax-wise yes. But the way I propose (and POC does already) is basically this:

salt().module().function()

So it will load only when you actually access it by calling and loading. Some internal Python tricks allows to get rid of () syntax, so it looks to the user like he would be just accessing already pre-loaded structure. However, this bonus feature was only an abstract example how we can build something on top of this principle.

Needless to say, this is not any new. The entire Zope is basically built on top of that. 😉

Also if this augmented the sysmod functionality that would be a good bonus.
https://docs.saltstack.com/en/latest/ref/modules/all/salt.modules.sysmod.html

Primarily we could get rid of foo['key']['otherkey']['somethingelse'] syntax everywhere in SLS. Especially when you want something like to make sure it won't fail in the middle:

if foo.get('key', {}).get('otherkey', {}).get('somethingelse'):
    ...

Much nicer to do just:

if foo.key.otherkey.somethingelse:
    ...

But as I said, this is all ideas for the future. Right now important is — capabilities.

@cachedout cachedout merged commit 26cab48 into saltstack:develop Dec 12, 2018
4 of 11 checks passed
@waynew waynew added this to PR needs port to master in PRs to port to master Oct 24, 2019
alexey-zhukovin pushed a commit to alexey-zhukovin/salt that referenced this issue May 1, 2020
@alexey-zhukovin alexey-zhukovin added the has master-port label May 1, 2020
@alexey-zhukovin alexey-zhukovin moved this from PR needs port to master to PR has port to master in PRs to port to master May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has master-port ZRETIRED - RFC
Projects
PRs to port to master
  
PR has port to master
Development

Successfully merging this pull request may close these issues.

None yet

6 participants