fix: check for double definition by blakepettersson · Pull Request #15670 · argoproj/argo-cd

blakepettersson · 2023-09-26T08:23:04Z

As found in #13965 (and as a follow-up to #13999), we also need to define what happens if both managedNamespaceMetadata and an Application manifest are defined for the same namespace.

The idea here is that if that happens, we emit an
ApplicationConditionRepeatedResourceWarning, and set the sync status to Unknown, since it's unclear what is supposed to happen. the managedNs doesn't get added to the list of targetObjs if there is already a source definition of the namespace.

Fixes #15815

Checklist:

As found in argoproj#13965 (and as a follow-up to argoproj#13999), we also need to define what happens if _both_ managedNamespaceMetadata _and_ an Application manifest are both defined for the same namespace. The idea here is that if that happens, we emit an `ApplicationConditionRepeatedResourceWarning`, and set the sync status to `Unknown`, since it's unclear what is supposed to happen. The user will then have the option of removing one of the two definitions. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

controller/state.go

codecov · 2023-09-26T08:37:58Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (129cf53) 50.01% compared to head (8b5a2e9) 49.62%.
Report is 90 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #15670      +/-   ##
==========================================
- Coverage   50.01%   49.62%   -0.40%     
==========================================
  Files         266      267       +1     
  Lines       45971    46438     +467     
==========================================
+ Hits        22992    23044      +52     
- Misses      20731    21133     +402     
- Partials     2248     2261      +13

Files	Coverage Δ
controller/state.go	`71.20% <100.00%> (+0.18%)`	⬆️

... and 32 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

blakepettersson · 2023-09-26T10:45:42Z

Looking at the test failure, and revisiting the rationale in #10672 for why the things are the way they are, it's probably better to ensure that the managedNs doesn't get added to the list of targetObjs if there is already a source definition of the namespace.

Will take a deeper look shortly.

A simpler fix - don't add a managed namespace to the targetObjs list if a namespace already exists in the application source. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

crenshaw-dev · 2023-10-10T15:15:52Z

@blakepettersson should we add a test for this?

This adds a test case showing that an ns manifest will override `managedNamespaceMetadata` without failing horribly (as it currently does on `HEAD`), as well as a "standard" mNMd diff vs live. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

blakepettersson · 2023-10-18T13:14:26Z

@crenshaw-dev I added a unit test for this, and #15852 would cover the e2e bit (once this gets merged)

blakepettersson · 2023-10-20T13:01:51Z

@crenshaw-dev this has come in with #15852, but it might be worthwhile backporting this to 2.8 / 2.9?

evenh · 2023-10-31T08:24:26Z

I would love a backport for this as we're currently on 2.8.4 🙏🏻

ishitasequeira

LGTM!!

ishitasequeira · 2023-11-08T03:04:48Z

/cherry-pick release-2.9

ishitasequeira · 2023-11-08T03:04:52Z

/cherry-pick release-2.8

* fix: check for double definition As found in #13965 (and as a follow-up to #13999), we also need to define what happens if _both_ managedNamespaceMetadata _and_ an Application manifest are both defined for the same namespace. The idea here is that if that happens, we emit an `ApplicationConditionRepeatedResourceWarning`, and set the sync status to `Unknown`, since it's unclear what is supposed to happen. The user will then have the option of removing one of the two definitions. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * fix: check for double definition A simpler fix - don't add a managed namespace to the targetObjs list if a namespace already exists in the application source. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * test: add test cases This adds a test case showing that an ns manifest will override `managedNamespaceMetadata` without failing horribly (as it currently does on `HEAD`), as well as a "standard" mNMd diff vs live. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

blakepettersson · 2023-11-08T10:45:21Z

Thanks a lot @ishitasequeira! Seems like there is some issue when it got merged though, fixed by #16275

* fix: check for double definition As found in #13965 (and as a follow-up to #13999), we also need to define what happens if _both_ managedNamespaceMetadata _and_ an Application manifest are both defined for the same namespace. The idea here is that if that happens, we emit an `ApplicationConditionRepeatedResourceWarning`, and set the sync status to `Unknown`, since it's unclear what is supposed to happen. The user will then have the option of removing one of the two definitions. * fix: check for double definition A simpler fix - don't add a managed namespace to the targetObjs list if a namespace already exists in the application source. * test: add test cases This adds a test case showing that an ns manifest will override `managedNamespaceMetadata` without failing horribly (as it currently does on `HEAD`), as well as a "standard" mNMd diff vs live. --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>

* fix: check for double definition As found in argoproj#13965 (and as a follow-up to argoproj#13999), we also need to define what happens if _both_ managedNamespaceMetadata _and_ an Application manifest are both defined for the same namespace. The idea here is that if that happens, we emit an `ApplicationConditionRepeatedResourceWarning`, and set the sync status to `Unknown`, since it's unclear what is supposed to happen. The user will then have the option of removing one of the two definitions. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * fix: check for double definition A simpler fix - don't add a managed namespace to the targetObjs list if a namespace already exists in the application source. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * test: add test cases This adds a test case showing that an ns manifest will override `managedNamespaceMetadata` without failing horribly (as it currently does on `HEAD`), as well as a "standard" mNMd diff vs live. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> Signed-off-by: jmilic1 <70441727+jmilic1@users.noreply.github.com>

* fix: check for double definition As found in argoproj#13965 (and as a follow-up to argoproj#13999), we also need to define what happens if _both_ managedNamespaceMetadata _and_ an Application manifest are both defined for the same namespace. The idea here is that if that happens, we emit an `ApplicationConditionRepeatedResourceWarning`, and set the sync status to `Unknown`, since it's unclear what is supposed to happen. The user will then have the option of removing one of the two definitions. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * fix: check for double definition A simpler fix - don't add a managed namespace to the targetObjs list if a namespace already exists in the application source. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * test: add test cases This adds a test case showing that an ns manifest will override `managedNamespaceMetadata` without failing horribly (as it currently does on `HEAD`), as well as a "standard" mNMd diff vs live. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* fix: check for double definition As found in argoproj#13965 (and as a follow-up to argoproj#13999), we also need to define what happens if _both_ managedNamespaceMetadata _and_ an Application manifest are both defined for the same namespace. The idea here is that if that happens, we emit an `ApplicationConditionRepeatedResourceWarning`, and set the sync status to `Unknown`, since it's unclear what is supposed to happen. The user will then have the option of removing one of the two definitions. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * fix: check for double definition A simpler fix - don't add a managed namespace to the targetObjs list if a namespace already exists in the application source. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * test: add test cases This adds a test case showing that an ns manifest will override `managedNamespaceMetadata` without failing horribly (as it currently does on `HEAD`), as well as a "standard" mNMd diff vs live. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> Signed-off-by: Kevin Lyda <kevin@lyda.ie>

* fix: check for double definition As found in argoproj#13965 (and as a follow-up to argoproj#13999), we also need to define what happens if _both_ managedNamespaceMetadata _and_ an Application manifest are both defined for the same namespace. The idea here is that if that happens, we emit an `ApplicationConditionRepeatedResourceWarning`, and set the sync status to `Unknown`, since it's unclear what is supposed to happen. The user will then have the option of removing one of the two definitions. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * fix: check for double definition A simpler fix - don't add a managed namespace to the targetObjs list if a namespace already exists in the application source. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * test: add test cases This adds a test case showing that an ns manifest will override `managedNamespaceMetadata` without failing horribly (as it currently does on `HEAD`), as well as a "standard" mNMd diff vs live. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

blakepettersson requested a review from crenshaw-dev September 26, 2023 08:23

blakepettersson commented Sep 26, 2023

View reviewed changes

controller/state.go Outdated Show resolved Hide resolved

fix: check for double definition

ab87d52

A simpler fix - don't add a managed namespace to the targetObjs list if a namespace already exists in the application source. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

blakepettersson marked this pull request as ready for review September 26, 2023 11:09

blakepettersson requested a review from a team as a code owner September 26, 2023 11:09

blakepettersson mentioned this pull request Sep 27, 2023

chore(deps): upgrade k8s version and client-go #13965

Closed

13 tasks

test: add test cases

8b5a2e9

This adds a test case showing that an ns manifest will override `managedNamespaceMetadata` without failing horribly (as it currently does on `HEAD`), as well as a "standard" mNMd diff vs live. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

ishitasequeira approved these changes Nov 8, 2023

View reviewed changes

ishitasequeira merged commit d4dc155 into argoproj:master Nov 8, 2023

gcp-cherry-pick-bot bot mentioned this pull request Nov 8, 2023

fix: check for double definition (cherry-pick #15670) #16271

Merged

gcp-cherry-pick-bot bot mentioned this pull request Nov 8, 2023

fix: check for double definition (cherry-pick #15670) #16272

Merged

This was referenced Nov 8, 2023

fix: failing lint and unit test #16275

Merged

lint and unit test failure after #15670 #16278

Closed

blakepettersson mentioned this pull request Nov 14, 2023

fix: check for double definition cherry-pick release-2.9 #16335

Merged

13 tasks

blakepettersson mentioned this pull request Nov 23, 2023

fix: check for double definition cherry-pick release-2.8 #16435

Merged

13 tasks

blakepettersson deleted the fix/managed-namespace-vs-manifest-check branch October 25, 2024 14:07

blakepettersson mentioned this pull request Jan 23, 2025

REQUEST: Promotion to Approver for blakepettersson argoproj/argoproj#350

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: check for double definition#15670

fix: check for double definition#15670
ishitasequeira merged 3 commits intoargoproj:masterfrom
blakepettersson:fix/managed-namespace-vs-manifest-check

blakepettersson commented Sep 26, 2023 •

edited by crenshaw-dev

Loading

Uh oh!

Uh oh!

codecov bot commented Sep 26, 2023 •

edited

Loading

Uh oh!

blakepettersson commented Sep 26, 2023

Uh oh!

crenshaw-dev commented Oct 10, 2023

Uh oh!

blakepettersson commented Oct 18, 2023

Uh oh!

blakepettersson commented Oct 20, 2023

Uh oh!

evenh commented Oct 31, 2023

Uh oh!

ishitasequeira left a comment

Uh oh!

ishitasequeira commented Nov 8, 2023

Uh oh!

ishitasequeira commented Nov 8, 2023

Uh oh!

blakepettersson commented Nov 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

blakepettersson commented Sep 26, 2023 • edited by crenshaw-dev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blakepettersson commented Sep 26, 2023

Uh oh!

crenshaw-dev commented Oct 10, 2023

Uh oh!

blakepettersson commented Oct 18, 2023

Uh oh!

blakepettersson commented Oct 20, 2023

Uh oh!

evenh commented Oct 31, 2023

Uh oh!

ishitasequeira left a comment

Choose a reason for hiding this comment

Uh oh!

ishitasequeira commented Nov 8, 2023

Uh oh!

ishitasequeira commented Nov 8, 2023

Uh oh!

blakepettersson commented Nov 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

blakepettersson commented Sep 26, 2023 •

edited by crenshaw-dev

Loading

codecov bot commented Sep 26, 2023 •

edited

Loading