Add NamespaceManagement reconciliation #1687
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rizwana777
force-pushed
the
issue-6179
branch
15 times, most recently
from
c16e6e4 to
f353e2a
Compare
Rizwana777
force-pushed
the
issue-6179
branch
2 times, most recently
from
c7bcc4e to
ccecd06
Compare
jgwest
requested changes
Apr 10, 2025
Rizwana777
force-pushed
the
issue-6179
branch
3 times, most recently
from
0788240 to
0d67be6
Compare
Rizwana777
force-pushed
the
issue-6179
branch
3 times, most recently
from
8da9978 to
1495dbc
Compare
jgwest
requested changes
May 6, 2025
Rizwana777
force-pushed
the
issue-6179
branch
3 times, most recently
from
53e5090 to
c4a47fa
Compare
Rizwana777
force-pushed
the
issue-6179
branch
7 times, most recently
from
f6d3ea6 to
974290d
Compare
jgwest
requested changes
Jun 7, 2025
| allowedNamespaces := make(map[string]bool) | ||
| if argocd.Spec.NamespaceManagement != nil { | ||
| for _, nm := range argocd.Spec.NamespaceManagement { | ||
| allowedNamespaces[nm.Name] = nm.AllowManagedBy |
There was a problem hiding this comment.
nm.Name can be a pattern; this logic isn't correct when nm.Name is a pattern instead of a namespace name.
| // Only act on namespaces in ArgoCD.spec.namespaceManagement that are truly managed by this ArgoCD | ||
| for _, ns := range argocd.Spec.NamespaceManagement { | ||
| nsName := ns.Name | ||
| if !managedNamespaces[nsName] { |
There was a problem hiding this comment.
nsName can be a pattern, rather than just a name, so this logic likely isn't correct.
controllers/argocd/secret.go
Outdated
| allowedNamespaceMap := make(map[string]bool) | ||
| for _, entry := range cr.Spec.NamespaceManagement { | ||
| if entry.AllowManagedBy { | ||
| allowedNamespaceMap[entry.Name] = true |
There was a problem hiding this comment.
entry.Name can be a pattern, not just a name. So this logic is likely not correct.
controllers/argocd/util.go
Outdated
|
|
||
| for _, ns := range oldList { | ||
| oldNamespaces[ns.Name] = true | ||
| oldNamespaceSettings[ns.Name] = ns.AllowManagedBy // Store old setting |
There was a problem hiding this comment.
Given that ns.name can be a pattern, I think this logic isn't correct. (The function that calls this assumes that the []string result are actual namespace names)
| assert.Equal(t, "ErrorOccurred", reconciledCondition.Reason) | ||
| } | ||
|
|
||
| func TestHandleFeatureDisable_NoNamespaceManagement(t *testing.T) { |
There was a problem hiding this comment.
Could use a few more tests that verify the logic in disableNamespaceManagement
controllers/argocd/util.go
Outdated
| } | ||
|
|
||
| // getNamespacesToDelete determines which namespaces were removed or had allowManagedBy changed. | ||
| func getNamespacesToDelete(oldList, newList []argoproj.ManagedNamespaces) []string { |
There was a problem hiding this comment.
Could use some unit tests for getNamespacesToDelete
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Signed-off-by: Rizwana777 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Add NamespaceManagement reconciliation
Add NamespaceManagement CR
Add .Spec.NamespaceManagement field in ArgoCDSpec CR
Unit tests
e2e for glob match on .Spec.NamespaceManagement field
Moved
k8sClient, err := initK8sClient()
if err != nil {
setupLog.Error(err, "Failed to initialize Kubernetes client")
os.Exit(1)
}
to main.go and also passing k8sClient in ReconcileArgoCD object to use it in code to avoid these
Failed to initialize Kubernetes client {"error": "invalid configuration: no configuration has been provided, try setting KUBERNETES_MASTER environment variable", "errorCauses": [{"error": "no configuration has been provided, try setting KUBERNETES_MASTER environment variable"}]}errorsand also e2e tests was failing because of this, I found out that initializing k8sClient in main.go would be the better option
let me know if this method is incorrect and needs modification
JIRA - https://issues.redhat.com/browse/GITOPS-6179