Skip to content
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

feat: add option to enable argo-rollout ui extension #1554

Merged
merged 9 commits into from
Oct 16, 2024

Conversation

saumeya
Copy link
Collaborator

@saumeya saumeya commented Oct 3, 2024

What type of PR is this?

/kind enhancement

What does this PR do / why we need it:

Have you updated the necessary documentation?

  • Documentation update is required by this PR.
  • Documentation has been updated.

Which issue(s) this PR fixes:

Fixes #?

How to test changes / Special notes to the reviewer:

On a cluster install argo-rollouts -
kubectl create ns argo-rollouts
kubectl apply -n argo-rollouts -f https://github.com/argoproj/argo-rollouts/releases/latest/download/install.yaml
kubectl apply -n default -f https://github.com/argoproj/argo-rollouts/releases/latest/download/install.yaml

In the ArgoCD CR enable rollout extensions

spec: 
  server:
     enableRolloutsUI: true

Then in the argoCD UI, create an application

project: default
source:
  repoURL: https://github.com/saumeya/argocd-example-apps.git
  path: rollout-basic
  targetRevision: HEAD
destination:
  server: https://kubernetes.default.svc
  namespace: default

Git link - https://github.com/saumeya/argocd-example-apps.git
Path - rollout-basic

After the application is synced, you can click on the Rollout resource in the tree structure of the resources and see the Rollout Extension enabled in the UI -

Screenshot 2024-10-02 at 12 43 40 AM

To disable it, set the option the enableRolloutsUI: false . This restarts the server pod (takes a few seconds), also the UI browser needs to be reloaded, the plugin will disappear.

Note - Disabling the field in the CR is not removing the rollout extension - I have created an issue upstream and debugging it - argoproj-labs/rollout-extension#85

@svghadi
Copy link
Collaborator

svghadi commented Oct 4, 2024

What do you think about below CRD structure?
Instead of

spec: 
  enableArgoRolloutUI: true

Something like

spec: 
  rollouts:
    ui: true

I don't have lot of understanding on how argo-rollouts works and integrates with operator but could be beneficial if there are future plans to support rollouts via operator. Worth checking with @jgwest on this.

@jgwest
Copy link
Member

jgwest commented Oct 8, 2024

@svghadi @saumeya Good question, re: API name/location, I think there's no right answer here, just different opinions 😄.

What about:

spec:
  server:
    enableRolloutsUI: true

Since this change only affects Argo CD server AFAICT, this might be a good place to put it, rather than creating a new field under .spec or .spec.rollouts. (I worry about us adding too many new fields to .spec)

@svghadi
Copy link
Collaborator

svghadi commented Oct 8, 2024

Sounds good to me.

Signed-off-by: saumeya <[email protected]>
Signed-off-by: saumeya <[email protected]>
Copy link
Collaborator

@svghadi svghadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@@ -520,6 +520,9 @@ type ArgoCDServerSpec struct {
// Autoscale defines the autoscale options for the Argo CD Server component.
Autoscale ArgoCDServerAutoscaleSpec `json:"autoscale,omitempty"`

// EnableRolloutUI will add the argo rollout UI extension in ArgoCD Dashboard.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change this to ArgoRollouts from ArgoRollout

@@ -231,6 +231,12 @@ const (
// ArgoCDDefaultResourceInclusions is the default resource inclusions.
ArgoCDDefaultResourceInclusions = ""

// ArgoCDRolloutExtensionImage is the default image for ArgoCD Rollout Extension Installer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the controller is Argo Rollouts. So, we should change this to Argo Rollouts.

Signed-off-by: saumeya <[email protected]>
Signed-off-by: saumeya <[email protected]>
@iam-veeramalla iam-veeramalla merged commit 833a92e into argoproj-labs:master Oct 16, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants