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

introduce the start service collector #6

Merged

Conversation

stefanotorresi
Copy link
Collaborator

Here is a working draft. If it looks good for you, I will proceed with writing some tests for it.

Copy link
Contributor

@MalloZup MalloZup left a comment

Choose a reason for hiding this comment

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

Some minor things on naming.

collector/start_service/start_service.go Outdated Show resolved Hide resolved
collector/start_service/start_service.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@MalloZup MalloZup left a comment

Choose a reason for hiding this comment

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

some other comments but ok

@MalloZup
Copy link
Contributor

MalloZup commented Mar 5, 2020

looks good

@MalloZup
Copy link
Contributor

MalloZup commented Mar 5, 2020

@stefanotorresi also we should state in the documentation how we generate the things etc, but this is for the final review of the pr, sofar I think is good!

@stefanotorresi
Copy link
Collaborator Author

thanks for the feedback, @MalloZup! good round! I will get at it immediately and ping you once this is ready.

@stefanotorresi stefanotorresi force-pushed the feature/start-service-collector branch from dbcbd14 to 1151d3a Compare March 5, 2020 15:55
@stefanotorresi stefanotorresi marked this pull request as ready for review March 5, 2020 16:07
README.md Outdated
$ ./sap_host_exporter
INFO[0000] Serving metrics on 0.0.0.0:9680
```shell
./sap_host_exporter --sap-control-url http://foobarbaz:50013
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have here like ```./sap_host_exporter --sap-control-url http://SAP_INSTANCE_URL_AND_NUMBER:50013````

Copy link
Contributor

Choose a reason for hiding this comment

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

SAP_URL = 'http://host:port?wsdl'

Copy link
Collaborator Author

@stefanotorresi stefanotorresi Mar 5, 2020

Choose a reason for hiding this comment

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

hmm yeah I'll make it more explanatory

Copy link
Contributor

@MalloZup MalloZup left a comment

Choose a reason for hiding this comment

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

looks good! thx! I just left some minor comments

I will review it more in deep tomorrow since I'm starting to be tired 😁

@stefanotorresi
Copy link
Collaborator Author

@MalloZup can I merge this?

@MalloZup MalloZup merged commit 7c61e33 into SUSE:master Mar 6, 2020
@stefanotorresi stefanotorresi deleted the feature/start-service-collector branch March 6, 2020 11:32
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.

None yet

2 participants