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.

Sign up for GitHub

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

Jump to bottom

rbac: add permissions for SSP operator #210

Merged
merged 1 commit into from
Aug 12, 2019
Merged

rbac: add permissions for SSP operator #210

merged 1 commit into from
Aug 12, 2019

Conversation

ffromani
Copy link
Contributor

Due a to a rebase glitch, the RBAC permissions for the SSP-operator
to work with Prometheus Rule were not included in
#199

Signed-off-by: Francesco Romani fromani@redhat.com

@ffromani
Copy link
Contributor Author

still not sure it is enough to fix #209

djzager
djzager approved these changes Aug 12, 2019
View reviewed changes
Copy link
Contributor

@djzager djzager left a comment

Choose a reason for hiding this comment

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

This was probably my fault and I don't think it will fix #209 (unless maybe the HCO needs additional permissions to create|get|...|watch the KubevirtMetricsAggregation resource) but this looks good to me.

@ffromani
Copy link
Contributor Author

Please let's wait a bit to see what's missing to fix issue#209 before to merge this PR

@ffromani ffromani mentioned this pull request Aug 12, 2019
Closed
@ffromani
Copy link
Contributor Author

It seems that the script used in #209 was somehow out of sync, that explains the lack of the MetricsAggregation CRD. I think we should merge this PR: without the operator will run, but will fail to deploy the PrometheusRule which it is supposed to.

@ffromani
Copy link
Contributor Author

Wait, do I need to update the templates under templates, don't I?

@ffromani
rbac: add permissions for SSP operator
51d041e 
Due a to a rebase glitch, the RBAC permissions for the SSP-operator
to work with Prometheus Rule were not included in
kubevirt#199

Signed-off-by: Francesco Romani <fromani@redhat.com>
@ffromani
Copy link
Contributor Author

/retest

djzager
djzager suggested changes Aug 12, 2019
View reviewed changes
Copy link
Contributor

@djzager djzager left a comment

Choose a reason for hiding this comment

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

Need to update the CSV.

First the permissions: https://github.com/kubevirt/hyperconverged-cluster-operator/blob/master/templates/olm-catalog/kubevirt-hyperconverged/VERSION/kubevirt-hyperconverged-operator.VERSION.clusterserviceversion.yaml.in#L213-L312
Then add the CRD: https://github.com/kubevirt/hyperconverged-cluster-operator/blob/master/templates/olm-catalog/kubevirt-hyperconverged/VERSION/kubevirt-hyperconverged-operator.VERSION.clusterserviceversion.yaml.in#L504-L506

@rthallisey
Copy link

/retest

djzager
djzager approved these changes Aug 12, 2019
View reviewed changes
Copy link
Contributor

@djzager djzager left a comment

Choose a reason for hiding this comment

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

Other than #211 (review) LGTM.

@rthallisey rthallisey merged commit 4426cd4 into kubevirt:master Aug 12, 2019
@ffromani ffromani deleted the metrics-aggregation-fix branch August 13, 2019 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djzager djzager djzager approved these changes

@rthallisey rthallisey rthallisey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ffromani @rthallisey @djzager