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 support for sidecar container in controller and server #758

Merged
merged 4 commits into from
Aug 9, 2024

Conversation

iam-veeramalla
Copy link
Collaborator

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 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
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
Collaborator 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
Collaborator Author

/retest

@svghadi
Copy link
Member

svghadi commented Aug 8, 2024

/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
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
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
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
Copy link
Member

anandf commented Aug 8, 2024

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
Copy link
Member

svghadi commented Aug 8, 2024

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
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
Copy link
Collaborator Author

iam-veeramalla commented Aug 8, 2024

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.

@svghadi
Copy link
Member

svghadi commented Aug 9, 2024

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm label Aug 9, 2024
Copy link

openshift-ci bot commented Aug 9, 2024

[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

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
20 checks passed
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 <[email protected]>

* fix: failing test due to timeout

Signed-off-by: iam-veeramalla <[email protected]>

* fix: failing tests

Signed-off-by: iam-veeramalla <[email protected]>

* fix: manifests updates

Signed-off-by: iam-veeramalla <[email protected]>

---------

Signed-off-by: iam-veeramalla <[email protected]>
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