Insights config API #1245

tremes · 2022-08-10T06:15:24Z

This is the second part (with simpliefied config API) and follow-up of #1193 and the original enhancement openshift/enhancements#1037

openshift-ci · 2022-08-10T06:15:31Z

Hello @tremes! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard lgtm and approved labels this repository requires either:

bugzilla/valid-bug - applied if your PR references a valid bugzilla bug

OR

qe-approved, docs-approved, and px-approved - these labels can be applied by anyone in the openshift org via the /label <labelname> command.

Who should apply these qe/docs/px labels?

For a no-Feature-Freeze team who is merging a feature before code freeze, they need to get those labels applied to their api repo PR by the appropriate teams (i.e. qe, docs, px)
For a Feature Freeze (traditional) team who is merging a feature before FF, they can self-apply the labels (via /label commands), they are basically irrelevant for those teams
For a Feature Freeze team who is merging a feature after FF, the PR should be rejected barring an exception

config/v1/type_insights.go

config/v1/types_insights.go

tremes · 2022-08-25T10:20:14Z

/retest

sferich888 · 2022-08-25T12:06:08Z

/label px-approved

xJustin · 2022-08-26T07:55:41Z

/label docs-approved

JoelSpeed

Sorry, missed that we didn't have an explanation of the values on the enum, we should make sure to add that

config/v1/types_insights.go

JoelSpeed · 2022-08-26T09:16:30Z

/test verify

JoelSpeed · 2022-08-26T09:50:21Z

I've asked @tremes to do some manual testing of the API to check the openapi validation behaves as we expect. Otherwise this API LGTM

/lgtm
/assign @deads2k

JoaoFula · 2022-08-30T07:53:28Z

/label qe-approved

config/v1/0000_10_config-operator_01_insightsdatagather.crd.yaml

config/v1/types_insights.go

JoelSpeed

One quick nit on the CRD but otherwise LGTM, please update and squash down

config/v1alpha1/0000_10_config-operator_01_insightsdatagather.crd.yaml

JoelSpeed · 2022-08-31T13:09:53Z

/lgtm

David will need to give a final review and approve

/assigned @deads2k

config/v1alpha1/types_insights.go

deads2k · 2022-09-01T14:09:11Z

config/v1alpha1/types_insights.go

+	// disabledGatherers is a list of gatherers to be excluded from the gathering. All the gatherers can be disabled by providing "all" value.
+	// If all the gatherers are disabled, the Insights operator does not gather any data.
+	// The particular gatherers IDs can be found at https://github.com/openshift/insights-operator/blob/master/docs/gathered-data.md.
+	// An example of disabling gatherers looks like this: `disabledGatherers: ["clusterconfig/machine_configs", "workloads/workload_info"]`


the API is ok, but where does a user locate the list of possible gatherers?

This question has occured in openshift/enhancements#1037 (comment). I still think listing all the gatherers here is not a good approach (because it will likely change). Can we refer to oc get insightsoperators.operator.openshift.io cluster -o json | jq .status.gatherStatus.gatherers ?

I'm just hoping to get this doc updated to tell me where to find the list. I'd be ok with such a command.

// The particular gatherers IDs can be found at https://github.com/openshift/insights-operator/blob/master/docs/gathered-data.md.

Line 46 has a link to a doc with the IDs, that should be roughly sufficient but a command is even better

deads2k · 2022-09-02T14:12:21Z

/lgtm
/approve

deads2k · 2022-09-02T14:13:06Z

/hold

holding for the doc update telling a user where to find a list of gatherers. Feel free to release after that

openshift-ci · 2022-09-05T10:21:11Z

@tremes: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

JoelSpeed · 2022-09-05T10:35:23Z

/lgtm
/hold cancel

Docs updates look good, thanks

openshift-ci · 2022-09-05T10:35:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, JoelSpeed, tremes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from jwforres and mfojtik August 10, 2022 06:15

JoelSpeed reviewed Aug 16, 2022

View reviewed changes

config/v1/type_insights.go Show resolved Hide resolved

tremes force-pushed the insights-config branch from b98e2ea to d1af8d3 Compare August 23, 2022 09:20

JoelSpeed reviewed Aug 23, 2022

View reviewed changes

config/v1/type_insights.go Outdated Show resolved Hide resolved

JoelSpeed reviewed Aug 24, 2022

View reviewed changes

config/v1/types_insights.go Outdated Show resolved Hide resolved

tremes force-pushed the insights-config branch from 3652ecd to 686c874 Compare August 25, 2022 06:59

tremes force-pushed the insights-config branch from 686c874 to bee6275 Compare August 25, 2022 10:55

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Aug 25, 2022

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Aug 26, 2022

JoelSpeed reviewed Aug 26, 2022

View reviewed changes

config/v1/types_insights.go Outdated Show resolved Hide resolved

tremes force-pushed the insights-config branch from 11e432e to 7cf36fc Compare August 26, 2022 09:31

openshift-ci bot assigned deads2k and JoelSpeed Aug 26, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2022

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2022

tremes force-pushed the insights-config branch from 7cf36fc to 2410104 Compare August 29, 2022 11:13

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2022

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2022

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Aug 30, 2022

tremes force-pushed the insights-config branch from 2410104 to 66c72ab Compare August 30, 2022 09:52

JoelSpeed reviewed Aug 30, 2022

View reviewed changes

config/v1/0000_10_config-operator_01_insightsdatagather.crd.yaml Show resolved Hide resolved

tremes commented Aug 30, 2022

View reviewed changes

config/v1/types_insights.go Show resolved Hide resolved

JoelSpeed reviewed Aug 31, 2022

View reviewed changes

config/v1alpha1/0000_10_config-operator_01_insightsdatagather.crd.yaml Outdated Show resolved Hide resolved

tremes force-pushed the insights-config branch from 533df11 to 82a3dbd Compare August 31, 2022 13:08

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2022

deads2k reviewed Sep 1, 2022

View reviewed changes

config/v1alpha1/types_insights.go Outdated Show resolved Hide resolved

deads2k reviewed Sep 1, 2022

View reviewed changes

config/v1alpha1/types_insights.go Outdated Show resolved Hide resolved

deads2k reviewed Sep 1, 2022

View reviewed changes

tremes force-pushed the insights-config branch from 82a3dbd to b5ac3f3 Compare September 2, 2022 11:35

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2022

Insights Data Gather Config API

ab8e345

tremes force-pushed the insights-config branch from c4356ad to ab8e345 Compare September 5, 2022 10:01

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 5, 2022

openshift-merge-robot merged commit 66f8283 into openshift:master Sep 5, 2022

Insights config API #1245

Insights config API #1245

Uh oh!

Conversation

tremes commented Aug 10, 2022

Uh oh!

openshift-ci bot commented Aug 10, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tremes commented Aug 25, 2022

Uh oh!

sferich888 commented Aug 25, 2022

Uh oh!

xJustin commented Aug 26, 2022

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoelSpeed commented Aug 26, 2022

Uh oh!

JoelSpeed commented Aug 26, 2022

Uh oh!

JoaoFula commented Aug 30, 2022

Uh oh!

Uh oh!

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoelSpeed commented Aug 31, 2022

Uh oh!

Uh oh!

Uh oh!

deads2k Sep 1, 2022

Choose a reason for hiding this comment

Uh oh!

tremes Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

deads2k Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

deads2k commented Sep 2, 2022

Uh oh!

deads2k commented Sep 2, 2022

Uh oh!

openshift-ci bot commented Sep 5, 2022

Uh oh!

JoelSpeed commented Sep 5, 2022

Uh oh!

openshift-ci bot commented Sep 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants