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

[UI] App of Apps 'Go-To-Parent' Button on Application Details View #19236

Open
rhyswilliamsza opened this issue Jul 25, 2024 · 6 comments · May be fixed by #19489
Open

[UI] App of Apps 'Go-To-Parent' Button on Application Details View #19236

rhyswilliamsza opened this issue Jul 25, 2024 · 6 comments · May be fixed by #19489
Labels
bug/enhancement component:ui User interfaces bugs and enhancements enhancement New feature or request type:enhancement

Comments

@rhyswilliamsza
Copy link

rhyswilliamsza commented Jul 25, 2024

Summary

I would like to propose the addition of a 'Go-To-Parent' button, added to the Application Details page for all App-of-Apps applications. This conditional button would appear if the appLabelKey is found on the currently open application, representing that the opened application is owned & managed by a parent application.

It would look something like the below:
Screenshot 2024-07-25 at 16 16 33

Motivation

Navigating app-of-apps structures down is a breeze when navigating down the tree (using the image selector), however becomes challenging when wanting to navigate back up the tree. This feature solves this problem by adding a simple (go back) button to the top toolbar.

Proposal

We luckily have all the bits needed to make this happen! The existing /settings endpoint provides the selector currently used for app ownership (appLabelKey), which can be reverse engineered to compile a navigation URL to go 'back to' the parent.

Caveats

I foresee two caveats and I would appreciate any thoughts re: if they're (1) worth pursuing, and (2) possible solutions?

  1. I see that ArgoCD is adding support for multi-namespace applications/multi argo deploys. As far as I know, the current app label tag does not include a namespace as part of tracking, so it may be necessary to do some sort of 'lookup'. We can (hopefully!) circumnavigate this for now by dropping the namespace entirely and letting the default 'get application' flow figure it out. If this is deprecated, this feature will break.

  2. This technique wouldn't work if the system is setup to use annotation tracking instead of the default label tracking. We may need to work out a different way of providing the parent-child relationship to the frontend, though I am not 100% sure if this is worth the additional API work at this time (unless the annotation also includes the app name? I must check that!). In addition, there may also be a scenario - if using the default app.kubernetes.io/instance label - where a separate system tags our argo application. We'd likely need to build in some sort of check to detect whether the parent really exists before adding the button.

Please let me know what you think! I'm working on a proof of concept here, which I am keen to turn into a PR if this proposal is accepted 😃

@rhyswilliamsza rhyswilliamsza added the enhancement New feature or request label Jul 25, 2024
@agaudreault agaudreault added the component:ui User interfaces bugs and enhancements label Jul 25, 2024
@agaudreault
Copy link
Member

@rhyswilliamsza Take a look at the UI code. I think you can leverage the isAppOfAppsPattern property from

isAppOfAppsPattern?: boolean;
and it might just requires change to the UI assets. To find the parent, I think using the ownerReference might be more accurate than the tracking annotations. If I am not mistaken, the ownerReference will be set to the parent App.

@rhyswilliamsza
Copy link
Author

rhyswilliamsza commented Jul 25, 2024

Thanks, @agaudreault!

I think you can leverage the isAppOfAppsPattern property from

Unfortunately when I looked this didn't appear to be set for an application on the application details page, but I'll look into implementing it for this context, too 🦾

I think using the ownerReference might be more accurate

100%, I actually didn't even know this existed. I'll take a stab and make a PR if all goes to plan :)

edit: hmm, looks like ownerReference may only be attached if ApplicationSets are in use. I'll see what I can do here. I'd much rather use a more opinionated field than a label/annotation if I can, so this is a good path to follow - thanks!

@agaudreault
Copy link
Member

Ahhh that's true, app of apps won't set any owners reference field.

@rhyswilliamsza
Copy link
Author

rhyswilliamsza commented Aug 1, 2024

Hi @agaudreault

Do you perhaps have any guidance for me on the use of cross-namespace owners references by applications? After having a bit of a look, attaching cross-namespace owner refs seems to move against the K8s pattern (though this seems to already be happening in applicationset cases). The main limitation is due to the prohibition of cross-namespace depedencies.

image

Was the decision made to ignore this limitation in the context of ArgoCD? I think the main point of concern would be when two applications, with the same name, exist in different namespaces. The owner ref pattern should be able to distinguish due to the different UID, however how would ArgoCD determine the correct parent in its current form?

@agaudreault
Copy link
Member

@rhyswilliamsza as you said previously, ownerReference is only used when the Applications are managed from an ApplciationSet. Argo does not Set ownerReference when an Application "syncs" another resource of kind Application. In that case, the tracking method is used.

Since the tracking is something server-side, I think the UI will have no choice but to receive an object/model from the server with a property that indicates the parent application/reference, based on the tracking method.

Maybe it would be possible to use ownerReference as a new tracking method, but there a probably a lot of limitations and it is a much larger change. You could create a new issue/discussion for this question specifically.

@rhyswilliamsza
Copy link
Author

Thanks for the quick feedback! I understand, thanks for the clarification. For now, let me go down the route of attaching additional context via a dedicated property 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/enhancement component:ui User interfaces bugs and enhancements enhancement New feature or request type:enhancement
Projects
None yet
3 participants