NETOBSERV-939: Detect which dashboards to show based on ocp version #399
Conversation
|
@jotak: This pull request references NETOBSERV-939 which is a valid 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 kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Based on OCP version, the console plugin reconciler will configure a
list of dashboard names that are available, so the the plugin will
create links for them
- Read & compare versions
- Some refactoring:
- Permission vendor detection and Available API detection were two
mechanisms very similar: merge Permission vendor detection in
Available APIs
- Reading cluster info (id, version) can also considered as part of the discovery
mechanism: merge them all in a new ClusterInfo structure, that
provides the ClusterVersion stuff and the available APIs
jotak
force-pushed
the
add-dashboards-links
branch
from
fd90fd1
to
c51cbea
Compare
|
@jotak: This pull request references NETOBSERV-939 which is a valid 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 kubernetes/test-infra repository. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #399 +/- ##
==========================================
+ Coverage 55.49% 55.57% +0.07%
==========================================
Files 45 45
Lines 5874 5920 +46
==========================================
+ Hits 3260 3290 +30
- Misses 2393 2407 +14
- Partials 221 223 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. |
jotak
added
no-qe
pkg/cluster/dashboards_list.go
Outdated
| func (d *Dashboards) CheckClusterDashboards(ctx context.Context, clusterInfo *Info) { | ||
| if ok, err := clusterInfo.OpenShiftVersionIsAtLeast("4.15.0"); err != nil { | ||
| // Log error but do not fail: it's likely a bug in code, if the openshift version cannot be found | ||
| log.FromContext(ctx).Error(err, "Could not get available dashboards for this cluster version. Is it OpenShift?") |
There was a problem hiding this comment.
Nit: log info including err.Message instead ?
There was a problem hiding this comment.
if fact this function is only called for openshift (on the caller's side, it's inside a if c.HasOCPSecurity() block, which is our way to detect openshift). But I agree it could be made more explicit, let me see that...
|
New changes are detected. LGTM label has been removed. |
|
@jpinsonneau 90498ef => better like that? |
Even better ! Thanks @jotak |
|
PR needs rebase. 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. |
This PR is based on #386 - merge it first
Related console-plugin PR: - to come -
Based on OCP version, the console plugin reconciler will configure a
list of dashboard names that are available, so the the plugin will
create links for them
mechanisms very similar: merge Permission vendor detection in
Available APIs
mechanism: merge them all in a new ClusterInfo structure, that
provides the ClusterVersion stuff and the available APIs