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

Conditional Data Gathering for Insights Operator #837

Merged

Conversation

Serhii1011010
Copy link
Contributor

No description provided.

@openshift-ci openshift-ci bot requested review from hardys and Miciah July 16, 2021 11:18
@Serhii1011010
Copy link
Contributor Author

@sttts, @deads2k, could you please review the api?

wking added a commit to wking/cluster-version-operator that referenced this pull request Sep 23, 2021
Separated from the rest of the CVO stuff, because the insights folks
might want to use this too [1].

[1]: openshift/enhancements#837
wking added a commit to wking/cluster-version-operator that referenced this pull request Sep 24, 2021
Separated from the rest of the CVO stuff, because the insights folks
might want to use this too [1].

[1]: openshift/enhancements#837
wking added a commit to wking/cluster-version-operator that referenced this pull request Sep 24, 2021
Separated from the rest of the CVO stuff, because the insights folks
might want to use this too [1].

[1]: openshift/enhancements#837
wking added a commit to wking/cluster-version-operator that referenced this pull request Sep 25, 2021
Separated from the rest of the CVO stuff, because the insights folks
might want to use this too [1].

[1]: openshift/enhancements#837
wking added a commit to wking/cluster-version-operator that referenced this pull request Sep 25, 2021
Separated from the rest of the CVO stuff, because the insights folks
might want to use this too [1].

[1]: openshift/enhancements#837
wking added a commit to wking/cluster-version-operator that referenced this pull request Nov 5, 2021
Separated from the rest of the CVO stuff, because the insights folks
might want to use this too [1].

[1]: openshift/enhancements#837
wking added a commit to wking/cluster-version-operator that referenced this pull request Nov 9, 2021
Separated from the rest of the CVO stuff, because the insights folks
might want to use this too [1].

[1]: openshift/enhancements#837
wking added a commit to wking/cluster-version-operator that referenced this pull request Nov 9, 2021
Separated from the rest of the CVO stuff, because the insights folks
might want to use this too [1].

[1]: openshift/enhancements#837
@tremes
Copy link
Contributor

tremes commented Nov 24, 2021

There's one last discussion opened and it seems leaning toward current wording with "conditions" word.
/approve

@Serhii1011010 Serhii1011010 force-pushed the conditional-data-gathering branch from 371109a to 310df60 Compare December 1, 2021 09:57
In case GitHub is not available, we won't be able to add new rules, but the old ones would still be returned by
the service.

## Design Details
Copy link
Contributor

Choose a reason for hiding this comment

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

I am missing here few words about the authentication. I guess the Insights operator will have to authenticate (using the "cloud.openshift.com" token from the pull-secret) to the new service.

I think it would be also good to describe the communication between the operator and the service more. How often will we query the service? What will the service API return - will be there some metadata e.g version or some time of update of the conditions? I remember there were some discussions about caching so this would be good to mention as well IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some info about implementation. Do you think we need the authentication? The rules aren't secret after all

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure, I guess everything running in crc requires authentication. We can check with @tisnik.

Copy link

Choose a reason for hiding this comment

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

@tremes Officially it would be needed. This is two-way thing as it allows the cluster (operator) to trust with whom it communicates (so not only for us to trust the cluster)

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tremes

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2021
Serhii Zakharov added 2 commits December 16, 2021 10:26
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 16, 2021

@Sergey1011010: all tests passed!

Full PR test history. Your PR dashboard.

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.

@tremes
Copy link
Contributor

tremes commented Dec 16, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2021
@openshift-merge-robot openshift-merge-robot merged commit 8251b1a into openshift:master Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.