Skip to content
This repository has been archived by the owner on May 30, 2022. It is now read-only.

Implement custom sapcontrol webservices #81

Conversation

fabriziosestito
Copy link
Member

@fabriziosestito fabriziosestito commented Jun 11, 2021

This PR decouples trento from sap_host_exporter, introducing trento/internal/sapsystems/sapcontrol containing a stripped-down version of the gowsdl generated soap webservice.
Here the sap_host_exporter original code for reference: https://github.com/SUSE/sap_host_exporter/tree/master/lib/sapcontrol

Webservice and soap client initialization has been moved away from internal/sapsystem to the webservice constructor, since we are just using a unix socket here, removing the need to use a viper configuration object.
Adds mapstructure tags to webservice result structs.

It also refactors tests by removing sap_host_exporter dependency gomock, using the mockery generator to be consistent with the rest of the project.

Resolves #79

Some questions:
should we add and link the full generated gowsdl code as reference to the documentation (see for example here: https://github.com/SUSE/sap_host_exporter/blob/master/doc/development.md#sapcontrol-web-service)?

Are the generated json/xml tags in the webservice's structs necessary?

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Hi @fabriziosestito . Fantastic work!
Just added some few details.

About your questions. I think the xml annotations are needed to the marshalling, I'm not sure about the json ones. @stefanotorresi will know for sure.
If we can remove them, let's remove them.

Edit. Yes, add a link to the full autogenerated code, it will make easier for newcomers

internal/sapsystem/sapcontrol/webservice.go Outdated Show resolved Hide resolved
internal/sapsystem/sapcontrol/webservice.go Show resolved Hide resolved
@rtorrero
Copy link
Contributor

Great work @fabriziosestito ! Thanks for the PR, I didn't find anything worth commenting other than the small typo on internal/sapsystem/sapsystem_kvstore_test.go and what was already mentioned by @arbulu89 .

@fabriziosestito fabriziosestito marked this pull request as ready for review June 11, 2021 14:23
arbulu89
arbulu89 previously approved these changes Jun 11, 2021
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

@fabriziosestito Green light from my side. If you tested the changes in a real trento env, and it still works the same as before, we can merge

@fabriziosestito
Copy link
Member Author

@fabriziosestito Green light from my side. If you tested the changes in a real trento env, and it still works the same as before, we can merge

I was not sure how I should confidence-test against a real trento env, so I basically exported the contents of the consul kv store from the master/this branch and compared them.

master.json
custom_sapcontrol_webservices.json

Comparing the two shows that only the key names and time-related values are changed.

Here the diff -u output for reference:
diff.output.txt

You might need to re-approve the PR since I squashed the fixup commits.

@rtorrero
Copy link
Contributor

LGTM

@stefanotorresi stefanotorresi merged commit 2db87f0 into trento-project:main Jun 14, 2021
@stefanotorresi
Copy link
Member

great stuff!

@stefanotorresi stefanotorresi mentioned this pull request Jun 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Implement custom sapcontrol webservices
4 participants