tools, vtysh: add the cli write callback, unhide an NB show command

[mjstapp]

Add the cli_show (config write) callback when emitting the create or modify callback in the northbound template. These are pretty necessary, and if they're not in the template, they have to be hand-crafted. Also unhide the "show configuration running" cli that interrogates the NB: this is pretty darn useful when doing NB work, and it's hard to use if it's hidden.

The CI checkpatch script complains because I've added the bare copyright line to the auto-gen nb template file.

[mjstapp]

CI:rerun

[idryzhov]

I don't think this is correct. Config output is context-specific – sometimes we implement a single callback for the whole container and sometimes we implement it for leafs but not the container. We shouldn't blindly generate the code for all containers and leafs in a schema.

[mjstapp]

Sure - I was just trying to extend the example that's already there. It's useful (at least, I found it useful) to have the auto-gen tool emit the xpaths, and offer the core callbacks. I don't disagree that not every node needs a config-cli callback - but not every node needs all of the other callbacks either? I found that using the auto-generated template as a reference helped me see how the tooling was seeing changes I made to a model; perhaps that's more helpful to newbies than to real first-class yang-ers? But it was helpful, and it was easier to skip 'write' callbacks that I didn't need than to manually add them in.

I don't think this is correct. Config output is context-specific – sometimes we implement a single callback for the whole container and sometimes we implement it for leafs but not the container. We shouldn't blindly generate the code for all containers and leafs in a schema.

[idryzhov]

Actually all other generated callbacks are required. If they are not defined then daemon fails on initialization. But anyway your idea sounds reasonable. How about we add some comment before the callback definition mentioning that it's optional and can be deleted if not needed? Also, can we change cli_write to cli_show. Most (if not all) of the code uses something_cli_show as function names.

[mjstapp]

sure, I'll add a comment and push an update.

I'm glad you asked about that callback name - I did want to raise the question about the existing convention. I've found the "show" function name confusing: we really use "show" to mean "show operational state", to mean "show xxx" cli commands. These callbacks don't have anything to do with oper state - just the opposite - and now that we're trying to talk about incorporating state retrieval into the northbound, it seemed like an opportunity to try to make the naming less ambiguous. What do you think about that?

Actually all other generated callbacks are required. If they are not defined then daemon fails on initialization. But anyway your idea sounds reasonable. How about we add some comment before the callback definition mentioning that it's optional and can be deleted if not needed? Also, can we change cli_write to cli_show. Most (if not all) of the code uses something_cli_show as function names.

[mjstapp]

When I look at it, a comment per function is kind of a lot - would a single comment at the top of the output be enough, do you think? Or is it better to have each generated line get the comment? Here's an example:

{
	.xpath = "/frr-nexthop:frr-nexthop-group/nexthop-groups/frr-nexthops/nexthop/weight",
	.cbs = {
		.modify = frr_nexthop_group_nexthop_groups_frr_nexthops_nexthop_weight_modify,
		/* Note: this cli callback is optional; the cli output may not need to be done at each node. */
		.cli_show = cli_write_frr_nexthop_group_nexthop_groups_frr_nexthops_nexthop_weight,
		.destroy = frr_nexthop_group_nexthop_groups_frr_nexthops_nexthop_weight_destroy,
	}
},

Actually all other generated callbacks are required. If they are not defined then daemon fails on initialization. But anyway your idea sounds reasonable. How about we add some comment before the callback definition mentioning that it's optional and can be deleted if not needed? Also, can we change cli_write to cli_show. Most (if not all) of the code uses something_cli_show as function names.

[idryzhov]

Okay, cli_write naming sounds good, thanks. Regarding the comment, how about we put it inside the function prototype in generate_config_write_callback? You already have TODO: implement me. comment there, just mention that it is actually optional.

[mjstapp]

sounds good - I've pushed a change that updates the comment in the generated 'write' function.

[mjstapp]

retrying CI...

[mjstapp]

CI:rerun

[mjstapp]

Added a doc entry too, after some discussion at today's dev meeting.

[mjstapp]

Rebasing, let's see if we can get through CI now

[mjstapp]

CI:rerun

[mjstapp]

Hi @idryzhov I think CI has finally passed - the 'checkout' error is from the presence of the copyright string for the auto-generated skeleton file.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[mjstapp]

rebased to current master, resolved conflicts

[mjstapp]

@idryzhov any chance you could re-visit this one?

[idryzhov]

Hi Mark, sorry for the late response. I actually used this code to generate callbacks for my zebra conversion. It helps and overall works fine.

I have two suggestions:

1. Let's move cli_write to the end of the function name. Currently, all configuration callbacks look like: lib_interface_zebra_multicast_modify/create/etc and this new callback is cli_write_lib_interface_zebra_multicast. Would be better to have lib_interface_zebra_multicast_cli_write.
2. As we're actively working on mgmtd conversion, we now have configuration and CLI callbacks in different structures. For example, you can look at staticd - in static_nb.c we have configuration callbacks and in static_vty.c we have CLI callbacks. So it may be more convenient if the generator prints them as two different structures as well.

[mjstapp]

Thanks: I'll move the _cli_write to be a suffix, sure.

I didn't understand exactly what you'd like in your (2) though. It looks like staticd has two structs with the same name, in two different modules. that seems a little weird? but staticd aside, are you saying you'd like the tool to generate two module_info structs - one with cli_show, cli_cmp, and cli_show_end (if generated), and another with everything else that's generated?

[...] I have two suggestions:

1. Let's move `cli_write` to the end of the function name. Currently, all configuration callbacks look like: `lib_interface_zebra_multicast_modify/create/etc` and this new callback is `cli_write_lib_interface_zebra_multicast`. Would be better to have `lib_interface_zebra_multicast_cli_write`.

2. As we're actively working on mgmtd conversion, we now have configuration and CLI callbacks in different structures. For example, you can look at staticd - in `static_nb.c` we have configuration callbacks and in `static_vty.c` we have CLI callbacks. So it may be more convenient if the generator prints them as two different structures as well.

[idryzhov]

I agree the naming is confusing, the second one is going to be renamed to frr_staticd_cli_info. So yes, it would be convenient to have two separate structures - frr_daemon_info with config callbacks and frr_daemon_cli_info with cli_show/show_end/cmp callbacks.

[mjstapp]

ok, got that: do you want the cli_show_end and cli_cmp callbacks to be generated now? the current code is only doing the 'write' callback (and prototype, etc).

I agree the naming is confusing, the second one is going to be renamed to frr_staticd_cli_info. So yes, it would be convenient to have two separate structures - frr_daemon_info with config callbacks and frr_daemon_cli_info with cli_show/show_end/cmp callbacks.

[idryzhov]

No, I think cli_show is enough. Others are rarely used, so I think it would be less convenient to have them in the output.

[mjstapp]

argh - needed to finish rebasing, not leave a temp commit hanging out...

[mjstapp]

ok, what's present now should include those changes...

No, I think cli_show is enough. Others are rarely used, so I think it would be less convenient to have them in the output.

[mjstapp]

looks like CI has passed now?

[idryzhov]

@mjstapp everything is good now, thanks for the updates!

But we need to update yang_module_load(argv[0]) call on line 462 to yang_module_load(argv[0], NULL) due to recent changes in master. Otherwise it doesn't build.

[mjstapp]

and rebased again, to catch up with current master