fix: Fix duplicate cluster-level resources with namespace when printing during execution of CLI commands (issue #18669) #19830
Conversation
Signed-off-by: TheCoolDrop <[email protected]>
… and fail Signed-off-by: TheCoolDrop <[email protected]>
…sources In this commit many changes have been made in order to prevent double printing of cluster level resources. The bug was as follows: When Application contained a cluster-level resource with metadata.namespace field specified, then any operations which printed the Application resources with wide output format would print the cluster-level resources twice. The resource would be printed once with namespace and second time without namespace. The bug is caused by difference in how ArgoCD saves the list of Application resources in Application.status.resources field and in Application.status.operationState.syncResult.resources field. Both of these fields taken into consideration when printing, but in Application.status.resources field the cluster-level resources are written with namespace set to "", no matter what was specified in original YAMLs, while in second location they are saved with namespace fully written out. This prevented the existing mechanisms from detecting that specific cluster-level resource has already been printed. In this commit I have: - Implemented a new mechanism for retrieving resources from both lists which ensures that there are no duplications - Adapted all of the commands which printed resources (most of them just transitively relied on waitOnApplicationStatus, which actually printed) - Adapted the fake testing client to include a cluster-level resource in order to be able to test this behavior - Adapted the test of waitOnApplicationStatus to ensure that new behavior is as expected Signed-off-by: TheCoolDrop <[email protected]>
… in print During deduplication of cluster level resources, if resource representation from operationState.syncResult came earlier, then the message would have been discarded. The code has been adapted to take this into consideration by merging, and the test has been adapted to include the message. Signed-off-by: TheCoolDrop <[email protected]>
This reverts commit f274933. Signed-off-by: TheCoolDrop <[email protected]>
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
✅ Preview Environment created on Bunnyshell but will not be auto-deployedSee: Environment Details Available commands (reply to this comment):
|
| @@ -93,8 +93,7 @@ COPY ./test/container/entrypoint.sh /usr/local/bin | |||
| ARG UID | |||
|
|
|||
| # Prepare user configuration & build environments | |||
| RUN userdel -r ubuntu && \ | |||
There was a problem hiding this comment.
Why remove this?
There was a problem hiding this comment.
That was an intermittent change, which I will revert. The issue is that when I try to build locally it tells me that email spool, or similar, is missing for user, and thus can not be deleted.
I have commented about this issue here: #18917 (comment)
The PR was already closed sadly.
| groupVersion += (resource.Group + "/") | ||
| } | ||
| groupVersion += resource.Version | ||
| apiResourceList, err := discoveryClient.ServerResourcesForGroupVersion(groupVersion) |
There was a problem hiding this comment.
This call looks like it might be slow.
There was a problem hiding this comment.
Maybe, but this code path is only traversed from CLI commands, and how slow can it be? It boils down to few HTTP requests at most.
| for _, apiResource := range apiResourceList.APIResources { | ||
| if resource.Group == apiResource.Group && resource.Version == apiResource.Version && resource.Kind == apiResource.Kind { | ||
| if !apiResource.Namespaced { | ||
| resource.Namespace = "" |
There was a problem hiding this comment.
Can we update the backend to properly handle this case?
There was a problem hiding this comment.
I am unsure to what case are you refering. Could you please explain what you would like to have changed, and where?
There was a problem hiding this comment.
Essentially to not overrride the namespace in CLI, but make ArgoCD not set it in the first place for cluster resources, or unset it before returning data to CLI.
There was a problem hiding this comment.
That does make sense, and was part of my initial analysis. I will have to dig significantly deeper though, and it may take me up to next week ( since I am basically clueless about ArgoCD codebase ). My assumption currently is though that this happens in GitOps Engine library. I made a quite thorough analysis about the cause of the bug in the issue this PR belongs to.
|
Fix the tests, please. |
As per title there was an issue in CLI where:
Then during printing in CLI commands the resource would be printed out twice. This has now fixed in hopefully all code paths.
I am unsure if this change needs cherry-picking into older versions. I kindly ask the maintainers to guide me here.
Closes issue #18669
Checklist: