Skip to content

feat: add support for sidecar container in controller and server#758

Merged
openshift-merge-bot[bot] merged 4 commits into
redhat-developer:masterfrom
iam-veeramalla:sidecar
Aug 9, 2024
Merged

feat: add support for sidecar container in controller and server#758
openshift-merge-bot[bot] merged 4 commits into
redhat-developer:masterfrom
iam-veeramalla:sidecar

Conversation

@iam-veeramalla

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind enhancement
/kind documentation

What does this PR do / why we need it:
Adds support for Sidecar container in Controller and Server

Have you updated the necessary documentation?

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

Which issue(s) this PR fixes:
Adds support for Sidecar container in Controller and Server

How to test changes / Special notes to the reviewer:

  • Added the E2E tests to validate the feature.
    1-106_validate_appcontroller_initcontainers
    1-107_validate_server_initcontainers
    1-108_validate_appcontroller_sidecar
    1-109_validate_server_sidecar

Comment thread Makefile
Comment on lines +126 to +137
.PHONY: operator-sdk
OPERATOR_SDK ?= $(LOCALBIN)/operator-sdk
operator-sdk: ## Download operator-sdk locally if necessary.
ifeq (,$(wildcard $(OPERATOR_SDK)))
@{ \
set -e ;\
mkdir -p $(dir $(OPERATOR_SDK)) ;\
OS=$(shell go env GOOS) && ARCH=$(shell go env GOARCH) && \
curl -sSLo $(OPERATOR_SDK) https://github.com/operator-framework/operator-sdk/releases/download/$(OPERATOR_SDK_VERSION)/operator-sdk_$${OS}_$${ARCH} ;\
chmod +x $(OPERATOR_SDK) ;\
}
endif

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes @svghadi

@iam-veeramalla

Copy link
Copy Markdown
Contributor Author

/retest

@svghadi

svghadi commented Aug 8, 2024

Copy link
Copy Markdown
Member

/lgtm

@openshift-ci openshift-ci Bot added lgtm and removed lgtm labels Aug 8, 2024
@@ -1,3 +1,7 @@
apiVersion: kuttl.dev/v1beta1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@iam-veeramalla usual timeout for parallel tests is 1200s. By adding a timeout in this file, it'll be overridden. Is this intentional?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think, this test is failing due to mismatch in the order of the volume mounts.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, the test is passing now. We can probably get rid of the timeout

@anandf

anandf commented Aug 8, 2024

Copy link
Copy Markdown
Member

if the side car containers are dynamically injected by other operators (say istio), gitops-operator should ignore such updates right ? Are we handing that use case ?

@svghadi

svghadi commented Aug 8, 2024

Copy link
Copy Markdown
Member

if the side car containers are dynamically injected by other operators (say istio), gitops-operator should ignore such updates right ? Are we handing that use case ?

I don't think we have considered such use-cases where external apps will inject sidecar into operator.

@varshab1210

Copy link
Copy Markdown
Member

1-031_validate_toolchain/1-check | ArgoCD version mismatch. Should be v2.11.2+25f7504, is v2.11.6+089247d

@iam-veeramalla could you please update the toolchain test

@iam-veeramalla

iam-veeramalla commented Aug 8, 2024

Copy link
Copy Markdown
Contributor Author

if the side car containers are dynamically injected by other operators (say istio), gitops-operator should ignore such updates right ? Are we handing that use case ?

I don't think we have considered such use-cases where external apps will inject sidecar into operator.

That's a very good point @anandf , Thank you. However, that change is required in Argo CD Operator. This repository should not be impacted by the changes you suggested.

This PR only contains the API changes, Tests and Manifests changes which can be merged.

Once I get the suggested change in to the Argo CD Operator, we can just update the go.mod in this repository to pick the latest Argo CD Operator changes.

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>
Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>
Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>
Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>
@svghadi

svghadi commented Aug 9, 2024

Copy link
Copy Markdown
Member

/lgtm
/approve

@openshift-ci openshift-ci Bot added the lgtm label Aug 9, 2024
@openshift-ci

openshift-ci Bot commented Aug 9, 2024

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: svghadi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved label Aug 9, 2024
@openshift-merge-bot openshift-merge-bot Bot merged commit 990a1f9 into redhat-developer:master Aug 9, 2024
trdoyle81 pushed a commit to trdoyle81/gitops-operator that referenced this pull request Aug 13, 2024
…hat-developer#758)

* feat: add support for sidecar container in controller and server

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>

* fix: failing test due to timeout

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>

* fix: failing tests

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>

* fix: manifests updates

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>

---------

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants