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

ApplicationSet Name can no longer be more than 63 chars #15282

Closed
dyatlov opened this issue Aug 29, 2023 · 12 comments
Closed

ApplicationSet Name can no longer be more than 63 chars #15282

dyatlov opened this issue Aug 29, 2023 · 12 comments
Labels
bug Something isn't working regression Bug is a regression, should be handled with high priority

Comments

@dyatlov
Copy link

dyatlov commented Aug 29, 2023

This is a regression from 2.7.
This commit introduced it: 7192eab

A seemingly information-only label makes it impossible to have applicationsets with longer names because from now on that name is copied into the tracking label.

We'd appreciate if that was configurable to be able to turn it on / off or override etc.

@dyatlov dyatlov added the bug Something isn't working label Aug 29, 2023
@jannfis jannfis added the regression Bug is a regression, should be handled with high priority label Aug 30, 2023
@ericblackburn
Copy link
Contributor

ericblackburn commented Aug 30, 2023

Let's please get this prioritized. It is a complete road block.

Made a note in the argo-contributors slack channel to raise awareness.

@ericblackburn
Copy link
Contributor

ericblackburn commented Aug 30, 2023

Also, i agree with your comment here

I noted on the argo cli issue that they should just grab the info from the k8s appset object. The information is already stored there.

  • Edit this was wrong due to a bug that I opened a PR for

Given the original goal was to query for appsets via the argo cli, #14285, I think #13456 should either be removed or changed to annotations, the correct way to store information.

I believe the Argo product needs to avoid using labels at all cost to link argo resources based their names. Argo is meant to be a k8s deployment tool. It should prioritize good hygine and not introduce bad practices that can spread when others see code like this and copy pasta the idea. So, just having an On/Off switch would make Argo fragile and anti-k8s.

@todaywasawesome
Copy link
Contributor

Recommendation from @rbreeze is that we move to annotation tracking as @ericblackburn has suggested. Anyone want to make a PR?

@speedfl
Copy link
Contributor

speedfl commented Sep 1, 2023

Annotation tracking is also the recommended solution for app in any namespace.
https://argo-cd.readthedocs.io/en/stable/operator-manual/app-any-namespace/#switch-resource-tracking-method

I can start working on a PR today 👍

@rumstead
Copy link
Member

rumstead commented Sep 1, 2023

What is the plan for enabling the cli to query for the child applications with the annotation?

@ericblackburn
Copy link
Contributor

The argo cli query might need to be a separate initiative. We should fix the critical blocking bug first and design a nice to have cli query solution later.

@speedfl
Copy link
Contributor

speedfl commented Sep 1, 2023

@rumstead we can think about it later but here are some solution

  • Using jsonPath
argocd app list --filter='$.metadata.ownerReferences[?(@.kind=='ApplicationSet')].name=test-appset'
argocd app list --filter='$.metadata.annotations.['argocd.argoproj.io/appset-instance']=test-appset'

jq style could also be used (argocd already uses gojq)

  • Using a new field
argocd app list --appset-name=test-appset

an example is --app-namespace that was introduced for application in any namespace feature

  • Using a field-selector...

@dyatlov
Copy link
Author

dyatlov commented Sep 1, 2023

@speedfl does that imply that we have to load a full application list into client before filtering it? I would rather filter applications on server side before returning (might help with a further application pagination initiative)

@speedfl
Copy link
Contributor

speedfl commented Sep 1, 2023

You are right @dyatlov above proposals imply loading all applications on client side.

I was just pointing that we could find other solutions compared to the existing one which brings the 63 char regression

@rumstead
Copy link
Member

rumstead commented Sep 1, 2023

argocd app list --filter='$.metadata.ownerReferences[?(@.kind=='ApplicationSet')].name=test-appset'

We wouldn't even need to add the annotation in this case :)

@rumstead
Copy link
Member

rumstead commented Sep 7, 2023

based on the PR we can probably close this now?

@dyatlov dyatlov closed this as completed Sep 7, 2023
@jannfis
Copy link
Member

jannfis commented Sep 13, 2023

A fix for this has been released in v2.8.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working regression Bug is a regression, should be handled with high priority
Projects
None yet
Development

No branches or pull requests

6 participants