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: Extend sync-options annotation with Force=true (#525) #526

Closed
wants to merge 1 commit into from

Conversation

n9
Copy link

@n9 n9 commented May 30, 2023

Implementation for #525, #414, argoproj/argo-cd#5882

@n9 n9 force-pushed the feature/525 branch 2 times, most recently from 5ac7618 to 7f2ec27 Compare May 30, 2023 08:16
@sonarcloud
Copy link

sonarcloud bot commented May 30, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell C 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (187312f) 54.67% compared to head (4b3b25d) 54.68%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #526   +/-   ##
=======================================
  Coverage   54.67%   54.68%           
=======================================
  Files          41       41           
  Lines        4635     4636    +1     
=======================================
+ Hits         2534     2535    +1     
  Misses       1905     1905           
  Partials      196      196           
Files Changed Coverage Δ
pkg/sync/common/types.go 54.16% <ø> (ø)
pkg/sync/sync_context.go 74.10% <100.00%> (+0.02%) ⬆️

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

@n9
Copy link
Author

n9 commented May 30, 2023

@crenshaw-dev How would you like to refactor the method in order to reduce the Cognitive Complexity?

Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Is there any existing unit testing for Replace which we could replicate for Force?

@n9
Copy link
Author

n9 commented Jun 5, 2023

@crenshaw-dev I have found:

func TestSync_Replace(t *testing.T) {

But it seems to me that it just tests that the correct method is used.

How the "force" flag is tested? (The "force" mode already exists in ArgoCD. This PR adds just the ability to specify it via an annotation.)

@sidharthramesh
Copy link

Any updates on this?

@crenshaw-dev
Copy link
Member

@n9 good question... I'm not sure how resourceOps.GetLastResourceCommand(kube.GetResourceKey(tc.target) works, but basically I'm looking for a way to verify that, when the Force=true option is specified, the kubectl action is actually performed with --force enabled.

Signed-off-by: n9 <n9@users.noreply.github.com>
@sonarcloud
Copy link

sonarcloud bot commented Aug 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@n9
Copy link
Author

n9 commented Aug 17, 2023

@crenshaw-dev
resourceOps.GetLastResourceCommand does not contain flags (such as --force) only the raw command (such as apply or replace).

@n9 n9 marked this pull request as ready for review August 17, 2023 14:17
@crenshaw-dev
Copy link
Member

@n9 I believe the mock ApplyResource function could be modified to store that flag information.

func (r *MockResourceOps) ApplyResource(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, force, validate, serverSideApply bool, manager string) (string, error) {
	r.SetLastValidate(validate)
	r.SetLastServerSideApply(serverSideApply)
	r.SetLastServerSideApplyManager(manager)
	r.SetLastResourceCommand(kube.GetResourceKey(obj), "apply")
	command, ok := r.Commands[obj.GetName()]
	if !ok {
		return "", nil
	}

	return command.Output, command.Err
}

@mcoklica1
Copy link

Hey @n9 @crenshaw-dev, any updates on testing this and proceeding forward? IIUC you want to see if force flag is visible via ArgoCD logs when sync happens (application-controller pod)?

@n9
Copy link
Author

n9 commented Dec 5, 2023

Unfortunately, I have to work on other things. Feel free to complete this PR.

@kkk777-7
Copy link
Contributor

kkk777-7 commented Dec 9, 2023

@crenshaw-dev Implemented unit test. Could you please take a look at this PR when you have some time? Thank you.

@ChristianCiach
Copy link

This seems to be obsolete now that #560 has been merged, I think.

@n9 n9 closed this Apr 19, 2024
@ktzsolt
Copy link

ktzsolt commented May 10, 2024

Hi!

First of all, thank you for the change!

May I ask how (and when) will this be integrated into Argo CD? All I see for gitops-engine is that the latest release is 2 years old here https://pkg.go.dev/github.com/argoproj/gitops-engine?tab=versions
So I don't quite get the release management of this component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants