argo-cd icon indicating copy to clipboard operation
argo-cd copied to clipboard

Drop support for `applications.argoproj.io/app-name`

Open crenshaw-dev opened this issue 3 years ago • 5 comments

Summary

Remove support for old tracking label, applications.argoproj.io/app-name

Motivation

"applications.argoproj.io/app-name" is found once in the argo-cd repo:

	// LabelKeyLegacyApplicationName is the legacy label (v0.10 and below) and is superseded by 'app.kubernetes.io/instance'
	LabelKeyLegacyApplicationName = "applications.argoproj.io/app-name"

That's very old.

The constant is used in one place: SetAppInstanceLabel.

That function is called in only one place: SetAppInstance.

That function is called by groupObjsForDiff, which uses argoSettings.AppLabelKey for the key param, and by GenerateManifests, which uses q.AppLabelKey for the key param.

q.AppLabelKey is set in 4 places:

  • getLocalObjectsString
    • called by getLocalObjects
      • called by findandPrintDiff, which uses argoSettings.AppLabelKey
      • called by NewApplicationManifestsCommand, which uses argoSettings.AppLabelKey
    • called by NewApplicationSyncCommand, which uses argoSettings.AppLabelKey
  • getRepoObjs
    • called by CompareAppState, which uses appStateManager.settingsMgr.GetAppInstanceLabelKey(), which pulls it directly from application.instanceLabelKey in argocd-cm
  • GetManifests, which uses Server.settingsMgr.GetAppInstanceLabelKey(), which gets the value directly from the configmap
  • GetManifestsWithFiles, which does the same as GetManifests

I believe that is a full accounting of where the deprecated label was used. Basically, if that label isn't explicitly configured in argocd-cm, we're not using it in the code base.

Proposal

Let's remove support for this label in v3.0.

crenshaw-dev avatar Feb 02 '23 20:02 crenshaw-dev

Some history/context about this, applications.argoproj.io/app-name is a little special. It was our first tracking method and IIRC, the label actually makes its way down to the pod spec, and not just the top-level managed resource. There are a handful of Argo CD instances that might use this legacy labeling at Intuit, but migrating away from it might actually cause a redeploy of Pods.

But I can confidently say the only people that might be affected by removing this label is internal argo cd instances at Intuit. It's also easy to check to see which ones are impacted by looking at the ConfigMap of who is using this legacy key.

jessesuen avatar Feb 09 '23 01:02 jessesuen

@crenshaw-dev is there anything else we need to do for this issue apart from removing the extra code and tests that handle the case for LabelKeyLegacyApplicationName in SetAppInstanceLabel()? I do not see any other changes from a code perspective but we can also probably add a check to explicitly return app.kubernetes.io/instance in GetAppInstanceLabelKey() if the key in the configmap is applications.argoproj.io/app-name which would probably make it easier for migrating any user that's still using the old label? WDYT?

gdsoumya avatar Apr 12 '23 06:04 gdsoumya

That seems reasonable. We'd also want to log warnings that the user should update their configured tracking method. And we should have complete release notes explaining the risk that the upgrade might cause pod restarts.

crenshaw-dev avatar Apr 12 '23 13:04 crenshaw-dev

@crenshaw-dev this was reverted if I am not mistaken. Should we reopen as this will be related to https://github.com/argoproj/argo-cd/issues/13981

agaudreault avatar Oct 10 '24 15:10 agaudreault

Yep!

crenshaw-dev avatar Oct 10 '24 15:10 crenshaw-dev

Can we start by merging just a deprecation warning in the next 2.x release so that when 3.0 comes around it can be missing?

jsoref avatar Jan 09 '25 03:01 jsoref

Removing from 3.0 as per discussion with @crenshaw-dev

reggie-k avatar Mar 04 '25 08:03 reggie-k