COO-1553: feat: bind the tls profile with the ui plugin deployments#1056
COO-1553: feat: bind the tls profile with the ui plugin deployments#1056jgbernalp wants to merge 1 commit intorhobs:mainfrom
Conversation
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
|
@jgbernalp: This pull request references COO-1553 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgbernalp The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@jgbernalp: This pull request references COO-1553 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn 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. |
📝 WalkthroughWalkthroughThis pull request introduces TLS profile support to the UI plugin controller system. A new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jgbernalp
requested review from
simonpasquier
and removed request for
marioferh
| switch plugin.Spec.Type { | ||
| case uiv1alpha1.TypeDashboards: | ||
| return createDashboardsPluginInfo(plugin, namespace, plugin.Name, image) | ||
| pluginInfo, err = createDashboardsPluginInfo(plugin, namespace, plugin.Name, image) |
There was a problem hiding this comment.
(nit) I'd rather see an early exit in case of error.
| pluginInfo, err = createDashboardsPluginInfo(plugin, namespace, plugin.Name, image) | |
| pluginInfo, err = createDashboardsPluginInfo(plugin, namespace, plugin.Name, image) | |
| if err != nil { | |
| return nil, err | |
| } |
| return nil, fmt.Errorf("plugin type not supported: %s", plugin.Spec.Type) | ||
| } | ||
|
|
||
| if pluginInfo != nil { |
There was a problem hiding this comment.
related to my other comment above: if we return early on error, we don't have to check for nil pluginInfo.
| return nil, fmt.Errorf("failed to fetch TLS profile from cluster: %w", err) | ||
| } | ||
|
|
||
| cfg.UIPlugins.TLSProfile = initialTLSProfileSpec |
There was a problem hiding this comment.
it can be fixed later but it feels like the TLS Profile configuration should be injected by main() just like the other options.
3 participants
Fixes: COO-1553
This PR binds the cluster TLS profile to the plugins, we added a flag in the compatibility matrix as not all the plugins support the command line args yet. We will be setting the value when the upstream plugin images are ready. Then we can completely remove the check when all the plugins support the TLS configuration.