-
Notifications
You must be signed in to change notification settings - Fork 95
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
NO-JIRA: (feat) ingress certificates gather #917
Conversation
@rluders: This pull request explicitly references no jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rluders 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 |
9d139fc
to
6fda77b
Compare
I need to fix the unit tests for this one, but it can be reviewed. |
const ingressCertificatesLimits = 64 | ||
|
||
var ingressNamespaces = []string{ | ||
"openshift-ingress-operator", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean that it's possible (supported) to create an Ingress controller also in this namespace please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
} | ||
|
||
// Step 5: Add certificate info to the certificates list | ||
found := false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be better to use a map for storing the ControllerInfo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not remember it all here, but I get the certificates and relate them with the Controllers because sometimes (yes it is weird). Still, the certificate and the controllers are living in separated namespaces. I didn't find a way to identify it using the API, so the best approach I found was to get all the certificates and them, check if any controller is using them. Weird? Yes.
Namespace string `json:"namespace"` | ||
} | ||
|
||
func (g *Gatherer) GatherClusterIngressCertificates(ctx context.Context) ([]record.Record, []error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation is missing here please
@rluders: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
Reused in #945 |
This PR implements a new data enhancement to...
Categories
Sample Archive
path/to/sample_data.json
Documentation
path/to/documentation.md
Unit Tests
path/to/file_test.go
Privacy
Yes. There are no sensitive data in the newly collected information.
Changelog
Breaking Changes
Yes/No
References
https://issues.redhat.com/browse/???
https://access.redhat.com/solutions/???