salt module API discussion

[liangxin1300]

hi krig: Let's talk about crmsh API in salt module. From my view, origin crmsh has many options, part of which are not necessarily export to salt user. Except bootstrap, we can mapping these into salt firstly, which I thought are commonly used for HA:

configure(type, options, ...) type includes: primitive group clone ms bundle location colocation order property show delete

resource(type, options, ...) type includes: start stop demote promote restart cleanup status

node(type, options, ...) type includes: fence standby online status

cluster(type, options, ...) type includes: start stop disable enable health status

Does above define suitable?

Regards, xin

[krig]

Maybe also these:

corosync add-node del-node reload show status get set

Otherwise it sounds like a good basic set, yes.

[liangxin1300]

Or, does this way looks better?

configure_primitive configure_group configure_clone configure_ms configure_bundle configure_location configure_colocation configure_order configure_property configure_show configure_delete

resource_start resource_stop resource_demote resource_promote resource_restart resource_cleanup resource_status

...

Options for each types are different, integrate them into one function will cause this function huge, and the usage messages not easy to write just in one function. I think the second way is more simple and clear, the rule of API define is "word_word"

[krig]

Ah yes, I think I misunderstood your first proposal!

I agree, I prefer this second proposal too.

[nick-wang]

I have checked the code for reference, found it is more likely parser the salt parameters to run the corresponding crm command here. And many options can/will be implemented here.

Instead of using different functions like "configure_show, configure_property, etc ..." for different command, i would suggest to abstract the function like "def configure(*args, kwargs):" and "def resource(*args, *kwargs)",then maintain a big dict for different cmds. Or more ambitious like "def crm(args, kwargs)"

The dicts will look like: configure = { "show":{"doc":"list of show type, valid elements include:xxx", "valid_check": None, "DC_only"=False, "attrs"={"xml":False, "changed":True,}}, "property":{"doc":"xxx", "valid_check":func1 & func2, "DC_only"=True, "attrs"={"output_loglevel=":trace, "python_shell":False,}}, .... }

The "valid_check func" can be abstract like or using custom func: (In case run input, can simple return the corresponding polished dict element as a hint) def fun1(num): return len(kwargs) > num ...

I think maintain few unified func with a dict is easier that implement many functions. Cause you will copy&paste many similar function code for different cmd. Any thoughts on it? @liangxin1300 @krig

[nick-wang]

I can write a draft version in a pr for sure if it make sense.

[liangxin1300]

I think using abstract function is less friendly with users because they may remember more options, from my view, the define of APIs following specific rule is also a good index helper itself for user, also can show directly to users what we can do or what we have

[krig]

I would also prefer to have the API documentation in salt be as easy to read as possible.

Maybe we could even skip the levels of crmsh and just have each command directly. It is rarely a conflict.

So, commands like show, start, stop, promote. No need for the prefix in configure_show really...

[nick-wang]

Hmm... i may not explain myself clearly. I also agree with different API doc, what i am thinking is so many commands have similar usage. It will be easier to maintain/code with an unified abstract function, also can leave an exception entry for different thing. So in general, i won't change any usage and it is complete user awareness.

Wait me couple minutes, maybe hour... i will write a draft code in another pull request. Maybe my idea is not so maturity for all circumstance, but explain in code is much easier.

[liangxin1300]

fine:)

[krig]

Sounds good :)

[nick-wang]

Please refer to https://github.com/krig/salt-module-crmsh/pull/4 Please ignore the possible grammar errors but show in pull request, cause i write it in a hurry and not test yet. Thanks a lot:)