feat: Ability to create custom labels for namespaces created with syncOptions CreateNamespace by pasha-codefresh · Pull Request #443 · argoproj/gitops-engine

pasha-codefresh · 2022-08-11T20:44:38Z

Signed-off-by: pashavictorovich <pavel@codefresh.io>

codecov · 2022-08-11T20:50:58Z

Codecov Report

Merging #443 (df041e5) into master (2bc3fef) will increase coverage by 0.05%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master     #443      +/-   ##
==========================================
+ Coverage   55.22%   55.27%   +0.05%     
==========================================
  Files          41       41              
  Lines        4449     4452       +3     
==========================================
+ Hits         2457     2461       +4     
+ Misses       1803     1802       -1     
  Partials      189      189

Impacted Files	Coverage Δ
pkg/sync/sync_context.go	`72.20% <60.00%> (+0.21%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

crenshaw-dev · 2022-08-12T15:49:41Z

pkg/sync/sync_context.go

+				unstructuredObjCopy := unstructuredObj.DeepCopy()
+				if sc.namespaceCreator(unstructuredObjCopy) {
+					tasks = append(tasks, &syncTask{phase: common.SyncPhasePreSync, targetObj: unstructuredObjCopy, liveObj: nil})
+				}


My expectation as a user would be that when I set the createNamespaceMetadata field, all auto-created namespaces will be updated with that metadata.

What if in WithNamespaceCreation we accepted multiple namespaceModifier functions and run them in series where they are currently being run?

@crenshaw-dev Do you want to add ns metadata to the already created auto namespace?

yes, use case is

User creates an app with autocreate namespaces
User syncs the app, and namespaces are auto-created
User adds a new label to the createNamespaceMetadata.labels field
User syncs the app, and the label is applied to already-auto-created namespaces

Working on this

@ashutosh16 @crenshaw-dev this use case is covered now

https://github.com/argoproj/argo-cd/pull/10288/files#diff-627c32a4f31d0f97e6ddc1f19ab3add7fb7791e0d1b0503d78c6c049e8cb4865R260

when the app is going to be synced namespaceModifier will be called and labels will be added to namespace

pkg/sync/sync_context.go

Signed-off-by: pashavictorovich <pavel@codefresh.io>

crenshaw-dev

lgtm!

jannfis

I have a few questions

jannfis · 2022-08-16T13:39:29Z

pkg/sync/sync_context.go

+				unstructuredObjCopy := unstructuredObj.DeepCopy()
+				if sc.namespaceCreator(unstructuredObjCopy) {
+					tasks = append(tasks, &syncTask{phase: common.SyncPhasePreSync, targetObj: unstructuredObjCopy, liveObj: nil})
+				}


Is there a good reason for using a copy of unstructuredObj here instead of just using the original?

it is in same way for modifier, i decided to do in same way here for prevent issues, probably no
https://github.com/argoproj/gitops-engine/pull/443/files#diff-74e77a4722803a7d3f87b956ec81b82d3c5a0dd50de299922aa2208fec57e6e9R791

yes, it can be removed

jannfis · 2022-08-16T13:49:01Z

pkg/sync/sync_context_test.go

 	"encoding/json"
 	"errors"
 	"fmt"
+	errors2 "k8s.io/apimachinery/pkg/api/errors"


Just a nit: Could we please rename the import to apierrors or something more obvious than errors2?

yes, it was autogenerated, sry

jannfis · 2022-08-16T13:53:10Z

pkg/sync/sync_context.go


 // WithNamespaceCreation will create non-exist namespace
-func WithNamespaceCreation(createNamespace bool, namespaceModifier func(*unstructured.Unstructured) bool) SyncOpt {
+func WithNamespaceCreation(createNamespace bool, namespaceModifier func(*unstructured.Unstructured) bool, namespaceCreator func(*unstructured.Unstructured) bool) SyncOpt {


I think it makes sense to make a unique type of func (*unstructured.Unstructured) bool, as the method signature becomes a little unreadable :)

Not sure that it starts look better, maybe you can advice better naming

Signed-off-by: pashavictorovich <pavel@codefresh.io>

sonarqubecloud · 2022-08-16T14:32:09Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

jannfis · 2022-08-16T17:53:41Z

Also, looking at the appropriate code in Argo CD, is there a reason we need a new modifier function and do not reuse the existing one here: https://github.com/argoproj/argo-cd/blob/2d544b42c9c41be85a7fcb25535e3013d2ae3350/controller/sync.go#L243?

This would save us an API change in gitops-engine.

pasha-codefresh · 2022-08-16T18:44:54Z

because modifier is calling only if namespace already exist, this hook is not calling when namespace not exist
https://github.com/argoproj/gitops-engine/pull/443/files#diff-74e77a4722803a7d3f87b956ec81b82d3c5a0dd50de299922aa2208fec57e6e9R796

@jannfis

jannfis

Sorry for coming back so late.

LGTM.

…with syncOptions CreateNamespace (argoproj#443)" This reverts commit a56a803.

#455) * Revert "feat: Ability to create custom labels for namespaces created with syncOptions CreateNamespace (#443)" This reverts commit a56a803. * remove import Signed-off-by: pashavictorovich <pavel@codefresh.io> * fix test Signed-off-by: pashavictorovich <pavel@codefresh.io> Signed-off-by: pashavictorovich <pavel@codefresh.io>

This builds upon argoproj#443; the main difference here is that we keep the existing API and do a bit more logic in the downstream client to determine whether this namespace should be updated or not. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

namespace labels hook

246a5fb

Signed-off-by: pashavictorovich <pavel@codefresh.io>

pasha-codefresh changed the title ~~namespace labels hook~~ feat: Ability to create custom labels for namespaces created with syncOptions CreateNamespace Aug 11, 2022

pasha-codefresh mentioned this pull request Aug 11, 2022

Add ability to create custom labels for namespaces created with syncOptions CreateNamespace argoproj/argo-cd#7799

Closed

crenshaw-dev suggested changes Aug 12, 2022

View reviewed changes

ashutosh16 reviewed Aug 12, 2022

View reviewed changes

pkg/sync/sync_context.go Show resolved Hide resolved

add tests

22678f3

Signed-off-by: pashavictorovich <pavel@codefresh.io>

pasha-codefresh marked this pull request as ready for review August 16, 2022 11:21

fix test

bcbd1a6

Signed-off-by: pashavictorovich <pavel@codefresh.io>

pasha-codefresh requested review from ashutosh16 and crenshaw-dev and removed request for ashutosh16 August 16, 2022 11:32

crenshaw-dev approved these changes Aug 16, 2022

View reviewed changes

jannfis reviewed Aug 16, 2022

View reviewed changes

pasha-codefresh added 2 commits August 16, 2022 17:03

rename import

1e0406d

Signed-off-by: pashavictorovich <pavel@codefresh.io>

remove deep copy

df041e5

Signed-off-by: pashavictorovich <pavel@codefresh.io>

pasha-codefresh requested review from jannfis and removed request for ashutosh16 August 16, 2022 14:44

todaywasawesome mentioned this pull request Aug 16, 2022

feat: Ability to create custom labels for namespaces created with syncOptions CreateNamespace argoproj/argo-cd#10288

Closed

jannfis approved these changes Aug 18, 2022

View reviewed changes

jannfis merged commit a56a803 into argoproj:master Aug 18, 2022

pasha-codefresh added a commit to pasha-codefresh/gitops-engine that referenced this pull request Aug 23, 2022

Revert "feat: Ability to create custom labels for namespaces created …

0f2d12b

…with syncOptions CreateNamespace (argoproj#443)" This reverts commit a56a803.

pasha-codefresh mentioned this pull request Aug 23, 2022

Revert "feat: Ability to create custom labels for namespaces created … #455

Merged

blakepettersson mentioned this pull request Sep 22, 2022

Update namespace v2 #465

Merged

Conversation

pasha-codefresh commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crenshaw-dev left a comment

Choose a reason for hiding this comment

Uh oh!

jannfis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Aug 16, 2022

Uh oh!

jannfis commented Aug 16, 2022

Uh oh!

pasha-codefresh commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jannfis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pasha-codefresh commented Aug 11, 2022 •

edited

Loading

codecov bot commented Aug 11, 2022 •

edited

Loading

pasha-codefresh commented Aug 16, 2022 •

edited

Loading