kernelkit / infix

Linux :yellow_heart: NETCONF = Infix
https://kernelkit.org
GNU General Public License v2.0
52 stars 12 forks source link

Upgrade NETCONF Client from 2.2.0 to 3.1.1 #683

Closed axkar closed 1 month ago

axkar commented 1 month ago

Refactor NETCONF functions (get_data) to accommodate the upgrade in the netconf_client library within the Docker container image.

Other information

Checklist

Tick relevant boxes, this PR is-a or has-a:

axkar commented 1 month ago

It is probably easier to ask and comment here in the PR than in the issue #390 @troglobit @mattiaswal I have a question regarding get_config in netconf.py.

I think we should not leverage _get for that since it is used to fetch both operational and running (all config). ncc.get_config allows for specifying source (datastore), as far as I see in netconf_client.

see: https://github.com/ADTRAN/netconf_client/blob/main/netconf_client/ncclient.py https://github.com/ADTRAN/netconf_client/blob/main/netconf_client/rpc.py

troglobit commented 1 month ago

@ahmkar94 not sure I follow?

The method infamy/netconf.py:_get() is (and should!) only used locally in the Class by get() and get_config(). You are free to refactor infamy/netconf.py any way you see fit to reduce the amount of extra code we have to make future upgrades of ncclient easier :smiley: :hammer_and_wrench:

axkar commented 1 month ago

@ahmkar94 not sure I follow?

The method infamy/netconf.py:_get() is (and should!) only used locally in the Class by get() and get_config(). You are free to refactor infamy/netconf.py any way you see fit to reduce the amount of extra code we have to make future upgrades of ncclient easier 😃 🛠️

I meant that as you said _get is used by both get() and get_config(), but _get does not allow for the source parameter to be passed when get_config is used as a getter function.This way get_config will always fetche the default (running configuration) when calling it (as far as I understand) :)

def get_config(self, source="running", filter=None, with_defaults=None):
axkar commented 1 month ago

But I can make an additional commit for that anyways and we see then :)

troglobit commented 1 month ago

I can checkout your branch and push a new infamy docker image.

Container image updated to v2.0 on ghcr.io:

https://github.com/kernelkit/infix/pkgs/container/infix-test/283471275?tag=2.0

So you can restart your actions to get the tests working.

axkar commented 1 month ago

I can checkout your branch and push a new infamy docker image.

Container image updated to v2.0 on ghcr.io:

https://github.com/kernelkit/infix/pkgs/container/infix-test/283471275?tag=2.0

Thnx!

troglobit commented 1 month ago

I meant that as you said _get is used by both get() and get_config(), but _get does not allow for the source parameter to be passed when get_config is used as a getter function. This way get_config will always fetche the default (running configuration) when calling it (as far as I understand) :)

OK. Sometimes upgrading/replacing our own wrappers is not a 1:1 mapping to new upstream APIs, this may lead to more or less work to refactor in several steps.

troglobit commented 1 month ago

But I can make an additional commit for that anyways and we see then :)

Yes, please submit a proposal. Much easier to discuss around code :-)