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

SetBatch doesn't allow each Update/Delete/Replace to have different Option arguments #83

Open
wenovus opened this issue Jan 31, 2023 · 3 comments

Comments

@wenovus
Copy link
Contributor

wenovus commented Jan 31, 2023

The Option argument is only on SetBatch.Set, so it applies for all paths in the SetRequest:

func (sb *SetBatch) Set(ctx context.Context, c *Client, opts ...Option) (*Result, error) {
	req := &gpb.SetRequest{}
	for _, op := range sb.ops {
		path, err := resolvePath(op.path)
		if err != nil {
			return nil, err
		}
		if err := populateSetRequest(req, path, op.val, op.mode, op.config, opts...); err != nil {
			return nil, err
		}
	}
...

Since it's referenced in the for loop, these options are clearly per-path (e.g. ygnmi.WithSetFallbackEncoding() is a per-path option) but the current API doesn't allow for different options for different paths.

If we do move the option to the individual batch calls, however, there may be downstream implications for ondatra:
https://github.com/openconfig/ondatra/blob/3b28896095b89670014dfb197224a83a50a9b5f2/gnmi/gnmi.go#L372-L381

So the root problem seems to be some of the options are per-client, while for others it is per-path.

I would like to break Ondatra's API to allow for a per-path behaviour. The use case is to Batch set CLI+OC in the same SetRequest.

@DanG100 Have any thoughts on how to support this?

@wenovus
Copy link
Contributor Author

wenovus commented Jan 31, 2023

Actually for the CLI+OC use case the support for this is likely to be different (probably just a special case), so ygnmi.WithSetFallbackEncoding() is not necessary. However, since supporting this would break the API it's probably better to do this earlier than later to allow better Batch support.

@DanG100
Copy link
Contributor

DanG100 commented Jan 31, 2023

Is there a use case for this? I get that in theory this might be nice, but a user could a also create 2 set requests.

@wenovus
Copy link
Contributor Author

wenovus commented Jan 31, 2023

Is there a use case for this? I get that in theory this might be nice, but a user could a also create 2 set requests.

There is no use case right now for it. I think we can park this item. If the user wants transactional semantics then a single setrequest would be necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants