-
Notifications
You must be signed in to change notification settings - Fork 326
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
integrate openshift route plugin with rollouts manager #15
integrate openshift route plugin with rollouts manager #15
Conversation
Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>
controllers/configmap.go
Outdated
} | ||
desiredConfigMap.Data["trafficRouterPlugins"] = string(pluginString) | ||
|
||
actualConfiMap := &corev1.ConfigMap{} |
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.
typo
controllers/default.go
Outdated
@@ -17,4 +17,11 @@ const ( | |||
DefaultRolloutsNotificationSecretName = "argo-rollouts-notification-secret" | |||
// DefaultRolloutsServiceSelectorKey is key used by selector | |||
DefaultRolloutsSelectorKey = "app.kubernetes.io/name" | |||
|
|||
// OpenshiftRolloutPluginName is the plugin name for Openshift Route Plugin | |||
OpenshiftRolloutPluginName = "openshift-route-plugin" |
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.
Should it be OpenShift
?
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.
posted some minor comments. Otherwise looks ok to merge
Signed-off-by: ishitasequeira <[email protected]>
api/v1alpha1/argorollouts_types.go
Outdated
@@ -43,6 +43,9 @@ type RolloutManagerSpec struct { | |||
|
|||
// Version defines Argo Rollouts controller tag (optional) | |||
Version string `json:"version,omitempty"` | |||
|
|||
// DisableRoutePlugin defines if the Openshift Route Plugin should not be installed on Rollouts Controller | |||
DisableRoutePlugin bool `json:"disableRoutePlugin,omitempty"` |
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 am thinking of having this API extendable to avoid future incompatible changes. Also its better to have it as a pointer to differentiate between the zero value of this variable and the explicit value set to false.
How does the below structure look like.
plugins:
- openshiftRoutePlugin: # name of the plugin can be of format argoproj-labs/openshift-route-plugin
enabled: false # or true
location: https://example.com/path/to/rollout-plugin-traffic-controller # if no path is provided, default would be picked from `/plugin` directory.
So that later we have option to add plugins
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 like the idea of having a way to extend plugins. I am thinking of below format in that case, let me know what you think.
plugins:
- name: openshift-route-plugin
enabled: true
localtion: https://example.com/path/to/rollout-plugin-traffic-controller # if no path is provided, default would be picked from `/plugin` directory.
Also, I would like to know how do we prioritize the location value details as users can also change this value in ConfigMap. In the current implementation, we are creating a ConfigMap (if not present already) and updating the ConfigMap with the plugin location only if the ConfigMap does not already contain the plugin name and location. If the plugin information is already present, we do not make any changes (i.e. do not update the location). By adding the location to the rollout-manager spec, how do we prioritize which location value gets picked up?
Signed-off-by: ishitasequeira <[email protected]>
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.
/lgtm
Thanks @ishitasequeira
Signed-off-by: ishitasequeira <[email protected]>
Integrate Openshift Route Plugin with RolloutsManager.