-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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(cli): fix tracking annotation diff for non-namespaced resources (#9773) #13924
fix(cli): fix tracking annotation diff for non-namespaced resources (#9773) #13924
Conversation
b8efe9b
to
80fd0d9
Compare
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #13924 +/- ##
==========================================
- Coverage 49.78% 49.64% -0.14%
==========================================
Files 261 258 -3
Lines 44672 44192 -480
==========================================
- Hits 22238 21940 -298
+ Misses 20249 20091 -158
+ Partials 2185 2161 -24
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the PR makes sense to me. Would you mind adding tests?
I can try, could you point me where it would be with an example I could base myself on, please? |
So, I think a unit test would be preferable. But our current examples of unit testing the CLI are very minimal and don't involve mocking a server (as you would have to do in this case). It might be tricky. It would probably involve modifying the Probably the easier way to go is to write an e2e test. There are already e2e tests which invoke the argo-cd/test/e2e/app_management_test.go Line 674 in 495d093
|
ecd2338
to
976ee9e
Compare
Thank you @crenshaw-dev for your guidance, I have added an E2E test, please let me know what you think. |
772e7ca
to
669e88b
Compare
Signed-off-by: Maxime Brunet <max@brnt.mx>
669e88b
to
dcdd1f1
Compare
/cherry-pick release-2.7 |
/cherry-pick release-2.8 |
Cherry-pick failed with |
…13924) Signed-off-by: Maxime Brunet <max@brnt.mx>
…rgoproj#13924) Signed-off-by: Maxime Brunet <max@brnt.mx> Signed-off-by: Jimmy Neville <jimmyeneville@gmail.com>
…rgoproj#13924) Signed-off-by: Maxime Brunet <max@brnt.mx>
…rgoproj#13924) Signed-off-by: Maxime Brunet <max@brnt.mx>
The
UnstructuredToAppInstanceValue()
function whichSetAppInstance()
passes the namespace to says the following:argo-cd/util/argo/resource_tracking.go
Lines 116 to 120 in b2c5901
But when called from the CLI, this namespace was always an empty string:
argo-cd/cmd/argocd/commands/app.go
Line 1122 in b2c5901
I have tested
argocd app diff
with--local
and--revision
, and they do not report the annotation diff anymore. I have not been able to test--server-side-generate
due to another bug (related to GRPC-Web in my setup: #12032), but I am confident it'll behave like the others. I have not tested withlabel
as tracking method (help welcome).Fixes #9773
Checklist:
Please see Contribution FAQs if you have questions about your pull-request.