feat: Add switch for Route and Service for Principal #1925
Conversation
jparsai
force-pushed
the
agent-service
branch
2 times, most recently
from
e9747bd to
c797448
Compare
jgwest
left a comment
jgwest
left a comment
There was a problem hiding this comment.
LGTM, some minor tweaks then good to go!
I'm assuming you already have, but if you haven't already, it would be good to run the updated E2E test on an OpenShift cluster to make sure it works as expected (since the E2E tests in argocd-operator repo use k3d which does not have Route installed).
|
|
||
| // ArgoCDAgentPrincipalServiceSpec defines the options for the Service backing the ArgoCD Agent Principal component. | ||
| type ArgoCDAgentPrincipalServiceSpec struct { | ||
| // Type is the ServiceType to use for the Service resource. |
There was a problem hiding this comment.
Can add a comment indicating what the default is for this field. Something like 'if type is not specified, the default of 'ClusterIP' will be used'
api/v1alpha1/argocd_types.go
Outdated
|
|
||
| // ArgoCDAgentPrincipalRouteSpec defines the options for the Route backing the ArgoCD Agent Principal component. | ||
| type ArgoCDAgentPrincipalRouteSpec struct { | ||
| // Enabled will toggle the creation of the OpenShift Route. |
There was a problem hiding this comment.
Can add a comment like, 'if enabled is not defined, the default is ...'
| // Env lets you specify environment for principal pods | ||
| Env []corev1.EnvVar `json:"env,omitempty"` | ||
|
|
||
| // Service defines the options for the Service backing the ArgoCD Agent component. |
There was a problem hiding this comment.
Can add a comment like: 'if Service is not defined, a default of ClusterIP will be used'
| // Service defines the options for the Service backing the ArgoCD Agent component. | ||
| Service ArgoCDAgentPrincipalServiceSpec `json:"service,omitempty"` | ||
|
|
||
| // Route defines the options for the Route backing the ArgoCD Agent component. |
There was a problem hiding this comment.
Can add a comment like 'If Route is not specified, a default of ... will be used'
|
|
||
| By("verifying Route for principal exists on OpenShift") | ||
| if shouldExpectRoute { | ||
| By("verifying Route for principal exists on OpenShift") |
There was a problem hiding this comment.
Nit: IMHO can move this By inside the if fixture.RunningOnOpenShift {},so as not to confuse the case where it is NOT running on openshift
jparsai
force-pushed
the
agent-service
branch
2 times, most recently
from
d458f26 to
62f651b
Compare
Thanks Jonathan, yes I tested all principal e2e tests in openshift. Addressed your review comments, PTAL. |
jparsai
force-pushed
the
agent-service
branch
from
8142a99 to
e656b3b
Compare
Assisted by: Cursor Signed-off-by: Jayendra Parsai <[email protected]>
jparsai
force-pushed
the
agent-service
branch
from
e656b3b to
2485cc0
Compare
2 participants
/kind enhancement
This PR adds a switch to enable/disable creation of principal route and option to provide expected type (ClusterIP/LoadBalancer) of principal service.
Fixes https://issues.redhat.com/browse/GITOPS-8072