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

AppInterface enhancements for subscription #67

Merged
merged 7 commits into from
Apr 25, 2023

Conversation

sachinholla
Copy link
Contributor

Added new translateSubscribe() and processSubscribe() functions to appInterface as per HLD sonic-net/SONiC#1287

Also added stub implementation of these functions to all existing app modules. It blindly treats all paths as non-db. Basic subscription features, without on_change, can be verified with this mapping.

Added new translateSubscribe() and processSubscribe() functions to
appInterface as per HLD sonic-net/SONiC#1287

Also added stub implementation of these functions to all existing app
modules. It blindly treats all paths as non-db. Basic subscription
features, without on_change, can be verified with this mapping.

Signed-off-by: Sachin Holla <sachin.holla@broadcom.com>
}

return nil, &notifInfo, nil
err := errors.New("Not supported")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why not

return tlerr.New("not implemented")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an existing code; but showing up here because gofmt converted indentation spaces into tabs. No idea why the original author chose to code it that way. I am not concentrating on such cleanups in this PR

translib/acl_app.go Outdated Show resolved Hide resolved
translib/acl_app.go Outdated Show resolved Hide resolved
translib/common_app.go Outdated Show resolved Hide resolved
translib/internal/apis/db_diff.go Show resolved Hide resolved
translib/pfm_app.go Show resolved Hide resolved
translib/sys_app.go Show resolved Hide resolved
translib/yanglib_app.go Show resolved Hide resolved
translib/subscribe_app_interface.go Show resolved Hide resolved
translib/subscribe_app_utils.go Outdated Show resolved Hide resolved
}

//App modules will use this function to register with App interface during boot up
// App modules will use this function to register with App interface during boot up
func register(path string, info *appInfo) error {
var err error
log.Info("Registering for path =", path)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While correcting this file, it would be helpful to convert stuff like this to a more explicit call like log.Infof("Registering for path =%s", path)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting to use Infof() instead of Info() ??
I am not performing any such cleanup of the existing code in this PR. I am only introducing translateSubscribe() & peocessSubscribe() methods and types used by them. All other changes you see (like line #98 here) are due to gofmt.

@sachinholla sachinholla requested a review from tomek-US April 21, 2023 04:25
Copy link

@tomek-US tomek-US left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for all the changes!

translib/internal/apis/notify.go Show resolved Hide resolved
translib/internal/apis/db_diff.go Show resolved Hide resolved
Copy link

@tomek-US tomek-US left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tomek-US tomek-US merged commit 6ce18c6 into sonic-net:master Apr 25, 2023
@sachinholla sachinholla deleted the subscribe_app_intf branch June 18, 2023 03:41
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

Successfully merging this pull request may close these issues.

3 participants