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

fix: check resource namespaces are managed #143

Merged
merged 2 commits into from
Sep 25, 2020

Conversation

jopit
Copy link
Contributor

@jopit jopit commented Sep 23, 2020

Signed-off-by: John Pitman jpitman@redhat.com

Related to argoproj/argo-cd#4329

Return an error from clusterCache.GetManagedLiveObjs if we find a target object in a namespace that is not managed by the cluster.

Signed-off-by: John Pitman <jpitman@redhat.com>
@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2020

Codecov Report

Merging #143 into master will increase coverage by 0.28%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
+ Coverage   53.79%   54.07%   +0.28%     
==========================================
  Files          25       25              
  Lines        2400     2415      +15     
==========================================
+ Hits         1291     1306      +15     
  Misses        974      974              
  Partials      135      135              
Impacted Files Coverage Δ
pkg/cache/cluster.go 49.87% <100.00%> (+1.45%) ⬆️
pkg/sync/sync_context.go 70.36% <0.00%> (+0.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d05efd...67e12e7. Read the comment docs.

return true
}
if len(namespace) == 0 {
return true
Copy link
Contributor

@alexmt alexmt Sep 23, 2020

Choose a reason for hiding this comment

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

I think we should return false here. If len(namespace) == 0 is true then resource is a cluster-level resource. Cluster level resources are not managed if cluster is connected in namespaced mode so we should return false.

if namespace == "" {
	return false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the suggested change.

Signed-off-by: John Pitman <jpitman@redhat.com>
@sonarcloud
Copy link

sonarcloud bot commented Sep 23, 2020

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

No Coverage information No Coverage information
51.1% 51.1% Duplication

@jopit
Copy link
Contributor Author

jopit commented Sep 24, 2020

Hi @alexmt the SonarCloud failure look like it's due to duplicated code in the tests I added. I think that eliminating the duplication will make the tests harder to understand. Do you want me to eliminate the duplication anyway?

Copy link
Contributor

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

LGTM

@alexmt alexmt merged commit 828dc75 into argoproj:master Sep 25, 2020
@alexmt
Copy link
Contributor

alexmt commented Sep 25, 2020

@jopit the SonarCloud check is optional. I think it is ok to have duplication in test

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.

None yet

3 participants