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

removed configsync dependency #455

Merged
merged 9 commits into from
Dec 5, 2023
Merged

Conversation

henderiw
Copy link
Contributor

@henderiw henderiw commented Dec 4, 2023

Removed the dependency to configsync namespace, but we need to see if the namespace is to be set.

@nephio-prow nephio-prow bot requested review from s3wong and tliron December 4, 2023 04:49
@henderiw
Copy link
Contributor Author

henderiw commented Dec 4, 2023

So this PR adds 2 changes to make it independent on configsync.

  1. the syncApp should be changed from configsync to tobeinstalledonremotecluster
  2. the remoteNamespace can be set. For Configsync this has to eb set.

@arora-sagar
Copy link
Contributor

@henderiw If I understand correctly I still have to create the secret in the management cluster right? Only then Nephio-Controller will act on it. In this case is it possible to add a possibility to populate the secret in the multiple clusters?

@henderiw
Copy link
Contributor Author

henderiw commented Dec 4, 2023

@henderiw If I understand correctly I still have to create the secret in the management cluster right? Only then Nephio-Controller will act on it. In this case is it possible to add a possibility to populate the secret in the multiple clusters?

right now there is a clusterName we lookup because we need to find the credentials of the remote cluster. I also believe it is bad practice to reuse a secret in multiple places, because a break means you have to update all of them.
So right now it is 1:1

@arora-sagar
Copy link
Contributor

@henderiw If I understand correctly I still have to create the secret in the management cluster right? Only then Nephio-Controller will act on it. In this case is it possible to add a possibility to populate the secret in the multiple clusters?

right now there is a clusterName we lookup because we need to find the credentials of the remote cluster. I also believe it is bad practice to reuse a secret in multiple places, because a break means you have to update all of them. So right now it is 1:1

But in skupper that's how a link between two sites is created, a secret is shared between clusters (same namespaces).

@henderiw
Copy link
Contributor Author

henderiw commented Dec 4, 2023

@arora-sagar I added logic to add the secret to a multiple clusters.

@efiacor
Copy link
Collaborator

efiacor commented Dec 5, 2023

/lgtm
/approve

@nephio-prow nephio-prow bot added the lgtm label Dec 5, 2023
@henderiw
Copy link
Contributor Author

henderiw commented Dec 5, 2023

/approve

Copy link
Contributor

nephio-prow bot commented Dec 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: efiacor, henderiw

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

@nephio-prow nephio-prow bot added the approved label Dec 5, 2023
@efiacor
Copy link
Collaborator

efiacor commented Dec 5, 2023

/lgtm

@nephio-prow nephio-prow bot added the lgtm label Dec 5, 2023
@nephio-prow nephio-prow bot merged commit 6f5745f into nephio-project:main Dec 5, 2023
PrimalPimmy pushed a commit to 5GSEC/nephio that referenced this pull request Aug 2, 2024
Removed the dependency to configsync namespace, but we need to see if
the namespace is to be set.
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.

3 participants