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

Add SSL certificate API. #59

Merged
merged 1 commit into from
May 21, 2021
Merged

Add SSL certificate API. #59

merged 1 commit into from
May 21, 2021

Conversation

ajarzyn
Copy link
Contributor

@ajarzyn ajarzyn commented Apr 25, 2021

Hello,

I've added few methods to from SSL certificate API.
I couldn't edit README (don't know why) thus example of usage here:


from synology_api import core_certificate


CertObj = core_certificate.Certificate('host', 'port',
                                       'login', 'password',
                                       secure=True, cert_verify=True)

cert_list = CertObj.list_cert()
old_certs = set()
for cert in cert_list['data']['certificates']:
    old_certs.add(cert['id'])

CertObj.upload_cert()
cert_list = CertObj.list_cert()
for cert in cert_list['data']['certificates']:
    if cert['id'] not in old_certs:
        CertObj.set_default_cert(cert['id'])
        break

This code import certificates to NAS, and make new imported cert as default.

I've also added parent object for core functionality not to duplicate to much code.
Because of that I edited files name for better indication of what application they are from.

Hope you will like changes.

Tested:
Certificate
SysInfo.storage()

@N4S4
Copy link
Owner

N4S4 commented Apr 26, 2021

Thank you @ajarzyn and great work, although there is gonna be some break changes in peoples code due to naming changes it is a good clean up and optimization, the thing I don't understand is why not include base_api_core into general auth without changing names, in that way the updated library won't break codes and won't be multiple auth modules, I think would be better. could you consider just adding module adding the base_api_core into general auth?

@ajarzyn
Copy link
Contributor Author

ajarzyn commented Apr 26, 2021

Hello @N4S4 I did not consider others people code at all, so thank you for pointing that out.

Idea behind base_api_core is that lot's of modules could base on Core but not all of them, and all of them will use Authentication but with different object, thus my proposition to implement it like this. Authentication does not depend on Core but Core depend on Authentication, just like every other "application".

Could you draft me a use case where change would brake others code?

As I understand base_api_core would not be imported by user but rather by API chunks like Certificate and others.

I could add a wrapper for ActiveBackup, Backup and sys_info, to ensure back compatibility, but I would suggest to put warning, that this object is depreciated and ask users to perform, proper changes.

From the other hand, it's python, changes can be quite dynamic right?

Please check this, here we can see that there are many different libraries for SYNO.Core, but also you can see other "applications" like SYNO.DSM, SYNO.Backup, and many more, thus I think it would be good to prepare this API for use case like this.

PS. Different aspect I wanted to talk about is making this API more flexible.
After short investigation it appears that REST API for synology product are quite similar. And synology router has quite similar API.

If you have any proposition please do not hositate to write them out.

Thanks.

@N4S4
Copy link
Owner

N4S4 commented Apr 27, 2021

It actually make sense, I was thinking just of the issue of renamed files would break code of somebody using it, but yes it is python. Improvements are always welcomed.

Honestly I never investigated about other synology products (as I cared and worked on my NAS) and would be interesting to implement other products API too as long there is going to be an available tester on that specific product. What products are you using?

I'll test your implementation and in that case I'll write in the change log the breaking changes for renamed files. At first look seems all good just give me a bit of time.

@ajarzyn
Copy link
Contributor Author

ajarzyn commented Apr 28, 2021

Hi sorry for long delay, it's hard to find spare moment.

I would like to propose refactoring/redesigning this module, to prepare it for what it may happen. This pull request shows that project structure could be better, and what is more important, there is no clear interface designed on which we could base new functionality.
I really like aerialls/synology-srm interface design but lack of POST implementation is a big issue for me. Please check it out and tell me what you think. There is also ProtoThis/python-synology, which is basically different implementation of API for Synology DSM (I have problem with understanding why there is so many different solution when everyone could contribute to one project making it grate).

I use RT2600ac and MR2200ac, but only RT2600ac is of interest here, as it's used as a main gate for all network.

Regarding file name changes we actually can make it step by step, implementing SSL functionality only, and create new branch with refactoring new interface could be better idea.

Making many different changes would cursed by all the good people using your module.
Thus maybe as I wrote before, it would be a good starting point to implement wrapper with note (log module would be nice here) that this class/method/function is depreciated.

@N4S4
Copy link
Owner

N4S4 commented Apr 30, 2021

It's ok I dont have much time too due to my job,
However I don't know how many people are using those modules (probably not many) and I don't know how much would affect they're use, I'd say that is better to write a wrapper with a deprecation notice until next updare to adapt to the new way to use which will be explained in the readme file, I think is the best way to proceed

@N4S4 N4S4 merged commit 13b85e4 into N4S4:master May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants