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

[Argo CD]: Add argocd AppProject-ApplicationSet-Application relationships #10920

Merged
merged 26 commits into from
May 22, 2024

Conversation

innocentrda
Copy link
Contributor

@innocentrda innocentrda commented May 10, 2024

Notes for Reviewers

This PR fixes #

This PR provides a Argo CD AppProject-ApplicationSet-Application relationships.

In Argo CD, The AppProject CRD is the Kubernetes resource object representing a logical grouping of applications. AppProject should exist before Applications are creatd within it. Similar to namespaces and pods in kubernetes.

ApplicationSet which uses a single manifest file to deploy multiple applications from one or multiple Git repositories to multiple clusters, should also reference AppProject to specify the project where generated Applications belong.

This become a Hierarchical-Parent relationship since AppProject should exist first.

Signed commits

  • Yes, I signed my commits.

@github-actions github-actions bot added component/server area/models Models, Components, Relationships related changes labels May 10, 2024
Copy link

github-actions bot commented May 10, 2024

Copy link
Member

@leecalcote leecalcote left a comment

Choose a reason for hiding this comment

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

Please add a description as to what the relationships are and justification for why they should exist.

"project"
]
],
"description": "Application can reference AppProject to specify the project it belongs to."
Copy link
Member

Choose a reason for hiding this comment

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

"can" or "does" or "should"?

"apiVersion": "core.meshery.io/v1alpha1",
"kind": "Hierarchical",
"metadata": {
"description": "A hierarchical inventory relationship in which the configuration of (parent) component is patched with the configuration of other (child) component. Eg: The configuration of the EnvoyFilter (parent) component is patched with the configuration as received from WASMFilter (child) component."
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we have a more readily comprehensible example for the Kubernetes layman as opposed to the somewhat advanced and niche EnvoyFilter example. A quick review of the rest of the existing policies might offer an alternative example such as Pod (child) and Namespace (parent).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hierarchical-Parent makes sense for this case if we can relate it to namespaces and pods. Project can be related to namespace.

Copy link
Member

Choose a reason for hiding this comment

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

Great. Given this, will you please update the sample text with a more readily comprehensible example for users?

Copy link
Member

@leecalcote leecalcote left a comment

Choose a reason for hiding this comment

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

Thanks for the updated description. Intuitively, a hierarchical-parent relationship makes much sense to me when I hear the word, "Set".

@leecalcote
Copy link
Member

leecalcote commented May 10, 2024

@innocentrda thanks for presenting this relationship on today's call. 👍 I'm curious as to what any one of the Argo maintainers think of this suggested relationship.

Copy link
Contributor

@MUzairS15 MUzairS15 left a comment

Choose a reason for hiding this comment

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

@innocentrda Will you use v1alpha2 version for this relationship definition?
Refer the existing defs for reference.

@MUzairS15
Copy link
Contributor

MUzairS15 commented May 13, 2024

Because there has been a recent change around schema version for these relationships (v1alpha2), so you might consider relocating to correct location.

"apiVersion": "core.meshery.io/v1alpha1",
"kind": "Hierarchical",
"metadata": {
"description": "A hierarchical inventory relationship in which the configuration of (parent) component is patched with the configuration of other (child) component. Eg: The configuration of the EnvoyFilter (parent) component is patched with the configuration as received from WASMFilter (child) component."
Copy link
Member

Choose a reason for hiding this comment

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

Great. Given this, will you please update the sample text with a more readily comprehensible example for users?

"apiVersion": "core.meshery.io/v1alpha1",
"kind": "Hierarchical",
"metadata": {
"description": "A parent-child relationship implies the requirement of the parent component before the child component can be created"
Copy link
Member

Choose a reason for hiding this comment

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

@innocentrda please take a quick moment to punctuate the first sentence and to include a second sentence using the pod and namespace example that has been suggested. Look to other relationships for text to copy/paste. There might be one of these already written.

Copy link
Member

Choose a reason for hiding this comment

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

We're missing a relationship-specific description. Will you re-add this so that users of this relationship have a description specific to AppSet and App?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@leecalcote The description is updated accordingly. Is it much better?

@@ -0,0 +1,45 @@
{
"apiVersion": "core.meshery.io/v1alpha2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for guidance. It is updated.

"schemaVersion": "core.meshery.io/v1alpha2",
"version": "v1.0.0",
"metadata": {
"description": "A parent-child relationship implies the requirement of the parent component before the child component can be created. For example, a Namespace in Kubernetes can be a parent of Pods within that namespace. Similarly, in Argo CD, an AppProject represents a logical grouping of Applications. Applications and ApplicationSets reference their AppProject by name."
Copy link
Member

Choose a reason for hiding this comment

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

Be sure to capitalize consistently, if you would: Namespace.

Yes, the description is ✅

MUzairS15
MUzairS15 previously approved these changes May 17, 2024
@leecalcote leecalcote merged commit 2ac9135 into meshery:master May 22, 2024
8 of 9 checks passed
Copy link

welcome bot commented May 22, 2024

Thanks for your contribution to Meshery! 🎉

Meshery Logo
        Join the community, if you haven't yet and please leave a ⭐ star on the project. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/models Models, Components, Relationships related changes component/server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants