Skip to content

Conversation

@dbenque-1a
Copy link
Contributor

Description of the change:
When creating the Service to expose metrics, use the NAMESPACE env variable instead of the WATCH_NAMESPACE that is empty for an operator generated with --cluster-scoped

Motivation for the change:
ExposeMetricsPort was failing to generate the kubernetes service to scrape metrics if the operator was generated with --cluster-scoped flag

Closes #835

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 12, 2018
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

This probably needs an e2e test case.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 13, 2018
Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Thanks for this! Few initial comments.

@lilic
Copy link
Member

lilic commented Dec 13, 2018

@hasbro17 had a good suggestion in #786 (comment) so pinging him on this.

@dbenque-1a dbenque-1a force-pushed the ExportServiceInClusterScopeOperator branch from 1aa79c0 to 48be906 Compare December 13, 2018 14:27
@joelanford
Copy link
Member

I think #786 (comment) makes a lot of sense.

I'd suggest we move this function to k8sutil.GetOperatorNamespace(), and then change this line to use it.

@dbenque-1a
Copy link
Contributor Author

@joelanford i will do that. Thanks.

@dbenque-1a dbenque-1a force-pushed the ExportServiceInClusterScopeOperator branch from 48be906 to 5b90a09 Compare December 13, 2018 22:20
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 13, 2018
@dbenque-1a
Copy link
Contributor Author

dbenque-1a commented Dec 13, 2018

@joelanford @lilic changes done according to your comment to reuse the proposal of #786 (comment)

@hasbro17, @estroz FYI

If it is accepted, should I squash all commits before the merge?

Description of the change:
When creating the Service to expose metrics, use the namespace of the operator instead of the WATCH_NAMESPACE

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

A couple of nits, but looks good to me overall.

Also, don't worry about squashing. That'll happen automatically when we merge the PR.

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Thanks for doing the changes!

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

One leftover comment otherwise lgtm!

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 17, 2018
@lilic
Copy link
Member

lilic commented Dec 17, 2018

pinging @estroz @joelanford to re-review. LGTM from my side.

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Thanks @dbenque-1a!

LGTM

@estroz
Copy link
Member

estroz commented Dec 17, 2018

@dbenque-1a needs a rebase. LGTM on green tests.

@dbenque-1a dbenque-1a force-pushed the ExportServiceInClusterScopeOperator branch from b906dbd to 321a7c9 Compare December 19, 2018 09:28
@dbenque-1a
Copy link
Contributor Author

@estroz, rebase done.
@estroz @lilic @joelanford , thanks for your reviews. At Amadeus ( https://amadeus.com/en | https://github.com/AmadeusITGroup/kubervisor ) we have already built couple of operators, the public ones are:
https://github.com/AmadeusITGroup/kubervisor
https://github.com/AmadeusITGroup/Redis-Operator
https://github.com/AmadeusITGroup/workflow-controller

I must say that this SDK should really improve our efficiency in that exercise. At some point we wanted to build such a SDK.. and you do it, thanks! So now we are going to be regular users and probably contributors.

Regards,
David Benque

@lilic
Copy link
Member

lilic commented Dec 19, 2018

I must say that this SDK should really improve our efficiency in that exercise. At some point we wanted to build such a SDK.. and you do it, thanks! So now we are going to be regular users and probably contributors.

Thanks for that! We are happy to welcome any contributions! And of course, feedback is always welcome.

I reran the tests they are 🍏 , this should be good to merge once @estroz dismisses his review. Thanks again!

@lilic lilic merged commit 94d4252 into operator-framework:master Dec 21, 2018
@lilic
Copy link
Member

lilic commented Dec 21, 2018

@dbenque-1a Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants