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

implemented fetching rules from a remote server for conditional gathering #583

Merged
merged 6 commits into from
Mar 21, 2022
Merged

implemented fetching rules from a remote server for conditional gathering #583

merged 6 commits into from
Mar 21, 2022

Conversation

Serhii1011010
Copy link
Contributor

@Serhii1011010 Serhii1011010 commented Feb 17, 2022 •
edited
Loading

Categories

  • Bugfix
  • Enhancement
  • Backporting
  • Others (CI, Infrastructure, Documentation)

Sample Archive

No changes

Documentation

  • README.md
  • docs/conditional-gatherer/README.md

Unit Tests

  • pkg/gatherers/conditional/conditional_gatherer_test.go
  • pkg/gatherers/conditional/parsing_test.go

Privacy

No new data was collected

Changelog

No

Breaking Changes

No

References

https://issues.redhat.com/browse/CCXDEV-7163
https://bugzilla.redhat.com/show_bug.cgi?id=???
https://access.redhat.com/solutions/???

@openshift-ci
Copy link

openshift-ci bot commented Feb 17, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 17, 2022
@openshift-ci openshift-ci bot requested review from mfojtik and rluders February 17, 2022 16:28
@openshift-ci
Copy link

openshift-ci bot commented Feb 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Sergey1011010

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 Feb 17, 2022
@Serhii1011010 Serhii1011010 force-pushed the fetch-rules-from-the-conditional-service branch from 5372a5f to 5325642 Compare February 17, 2022 16:31
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2022
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2022
@Serhii1011010 Serhii1011010 force-pushed the fetch-rules-from-the-conditional-service branch from ed8ff02 to 36e1625 Compare March 8, 2022 18:11
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2022
@Serhii1011010 Serhii1011010 force-pushed the fetch-rules-from-the-conditional-service branch from d284431 to 37a8a23 Compare March 9, 2022 17:08
@Serhii1011010 Serhii1011010 force-pushed the fetch-rules-from-the-conditional-service branch from 55079ec to a31e046 Compare March 11, 2022 14:47
@Serhii1011010 Serhii1011010 marked this pull request as ready for review March 11, 2022 14:48
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2022
@openshift-ci openshift-ci bot requested review from derekwaynecarr and tremes March 11, 2022 14:49
tremes
tremes reviewed Mar 14, 2022
View reviewed changes
pkg/gatherers/conditional/conditional_gatherer.go
},
},
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these conditions are not currently defined by the new service. Can you please create a task to update it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RedHatInsights/insights-operator-gathering-conditions#10
I'll also update the tag in the service once this one is merged to trigger a new build of the service

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The service update PR RedHatInsights/insights-operator-gathering-conditions-service#140

tremes
tremes reviewed Mar 15, 2022
View reviewed changes
pkg/gatherers/conditional/conditional_gatherer.go
return "", fmt.Errorf("gathering rules service client is nil")
}

rulesBytes, err := g.gatheringRulesServiceClient.RecvGatheringRules(ctx, config.ConditionalGathererEndpoint)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding some info log around here please? Something like fetching or pulling the conditions from endpoint bla bla bla?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought it would be good to put the endpoint URL in the log. Theoretically, it can happen that someone sets wrong endpoint and then it can help when troubleshooting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

tremes
tremes reviewed Mar 17, 2022
View reviewed changes
pkg/controller/controller.go Outdated
@@ -0,0 +1,58 @@
package controller
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename this file since it has nothing to do with controller right? Perhaps kubeconfigs.go or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@tremes
Copy link
Contributor

tremes commented Mar 17, 2022

I reviewed this one couple times. Good job. Thank you.

@tremes
Copy link
Contributor

tremes commented Mar 17, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2022
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

7 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@xJustin
Copy link
Contributor

xJustin commented Mar 18, 2022

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Mar 18, 2022
@JoaoFula
Copy link
Contributor

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Mar 18, 2022
@sferich888
Copy link

/label px-approved

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Mar 21, 2022
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link

openshift-ci bot commented Mar 21, 2022

@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.

@openshift-merge-robot openshift-merge-robot merged commit 0af2c91 into openshift:master Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tremes tremes tremes left review comments

@mfojtik mfojtik Awaiting requested review from mfojtik

@rluders rluders Awaiting requested review from rluders

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

Assignees

@tremes tremes

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Serhii1011010 @tremes @openshift-bot @xJustin @JoaoFula @sferich888 @openshift-merge-robot