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: make specifying config-management sidecar plugin image optional #1604

Merged

Conversation

jopit
Copy link
Collaborator

@jopit jopit commented Nov 19, 2024

What type of PR is this?

/kind enhancement

What does this PR do / why we need it:

Make specifying the sidecar image for a config-management plugin optional. When the image is not specified the operator will use the product image being used for the repo server.

Copy link
Member

@jgwest jgwest left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @jopit!

Two quick requests:

  • Can you add a short description of the new behaviour to docs/usage/config_management_2.0.md? This looks like the best place to document the change in the code itself.
  • Can you likewise add a short description of the behaviour as a comment to SidecarContainers field in
    // SidecarContainers defines the list of sidecar containers for the controller deployment
    (and also the v1alpha1 API).

(I presume we will ALSO be documenting this downstream, but it couldn't hurt to briefly include it here too.)

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.

@svghadi
Copy link
Collaborator

svghadi commented Dec 5, 2024

Fixes #923

@jopit
Copy link
Collaborator Author

jopit commented Dec 5, 2024

  • Can you likewise add a short description of the behaviour as a comment to SidecarContainers field in
    // SidecarContainers defines the list of sidecar containers for the controller deployment

The description should actually be added to the SidecarContainers field in the ArgoCDRepoSpec

// SidecarContainers defines the list of sidecar containers for the repo server deployment

@jopit jopit force-pushed the gitops-5879-sidecar-default-image branch from d66e6db to c437afb Compare December 5, 2024 14:14
jopit added 2 commits December 5, 2024 10:47
Signed-off-by: John Pitman <[email protected]>
Signed-off-by: John Pitman <[email protected]>
Copy link
Member

@jgwest jgwest left a comment

Choose a reason for hiding this comment

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

LGTM too, thanks again @jopit!

@jgwest jgwest merged commit fb03f78 into argoproj-labs:master Dec 9, 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.

3 participants