Use app label as app-change label for new apps

[praveenrewar]

What this PR does / why we need it:

• Introduce a new label kapp.k14s.io/app-change-app-label for app changes which uses the app label as value
• Use the old label to list and delete app-changes created for exiting apps, but also add this new label to them.
• Use the new label to list and delete app-changes created for new apps, but add the old label if length of app name is less than the maximum allowed length of a label key to keep the app-change backward compatible

Which issue(s) this PR fixes:

Fixes #646

Does this PR introduce a user-facing change?

Breaking change!!
App label generated by kapp will be used as app-change label instead of the app name for all the new apps.
- Using app name as a label value in app changes puts a limit on the number of characters a app name can have
- If an app deployed with new version of kapp (and having more than 63 characters) is used with an old version of kapp, then the old version won't be able to detect the app-changes created by the new version.

Additional Notes for your reviewer:

In future, after x number of releases, we can stop using the legacy label and start using the new label for existing apps as well. Since all the app-changes created for them would most probably have both the labels, it won't result in any loss of app-changes.

Review Checklist:

• Follows the developer guidelines
• Relevant tests are added or updated
• Relevant docs in this repo added or updated
• Relevant carvel.dev docs added or updated in a separate PR and there's
  a link to that PR
• Code is at least as readable and maintainable as it was before this
  change

Additional documentation e.g., Proposal, usage docs, etc.:

[100mik]

A few thoughts and I am thinking about whether migrated apps that use the label name today is being considered.

[100mik]

I see that you called out that we won't be any change for existing + migrated apps. That checks out.
The only sore point I have is this means that we will never entirely stop using the old way.
The changes seem to be in line with the behaviour that you are calling out.

What I was wondering is, since app-changes are something we garbage collect. Would it make sense to do this at an app-change level?

1. All new app-changes have app label
2. We query for both labels, unless the app-changes-use-app-label annotation is set.
3. We set the annotation when we get an empty list when looking for app-changes with the old label

So the huge downside is we make one more call till the app is migrated. And from a kapp-controller perspective. For stable packages that do not have diffs, this might be forever till they are reinstalled.

I am trying to figure out how we can potentially move to using just the new label. The path we are taking right now feels like we will be handling both cases for a long time since we cannot definitely said that no migrated or existing apps exist on the users cluster. And we end up leaving orphaned config maps when we do decide to move to the new one altogether.

Since, this is a case that is consciously not being handled in the PR. We can however track how we can "taper off" using old labels as a separate issue since this fix might be needed sooner rather than later.

TL;DR: The path to us not supporting the old label at all in the future is not clear. But we might track that separately and improve on the cases that we do not handle.
@praveenrewar thoughts?

[praveenrewar]

@100mik Thanks for bringing this up. I was planning to take that up separately, but now I think that we should at least decide how we migrate the app-changes for existing apps before making this change.

How about we add one more label to app-changes (not label value, but new label itself). For new apps we will use the new label itself, for old apps we will use the old label but also add the new label to the app changes. After some releases we will switch to using the new label only.

New app-changes for existing apps:

labels:
    kapp.k14s.io/app-change-app: foo
    kapp.k14s.io/app-change-app-label: 101920101920
    kapp.k14s.io/is-app-change: ""

New app-changes for new apps:

labels:
    kapp.k14s.io/app-change-app-label: 101920101920
    kapp.k14s.io/is-app-change: ""

Does this make sense? I will see how these changes looks from code perspective.

[100mik]

Yeah, I think adding both labels now does give us a part forward. We should do that if it is feasible 🤔

[100mik]

The changes look good to me, adding both labels leave more room for us to plan out the migration!
I was wondering if there is a straightforward way of putting together tests for this, but I believe we do not have any form of tests involving older versions of kapp. So I am happy with these changes as long as we have verified the tabulated test cases. Let me know if you need a hand with the verification!

[100mik]

The additional condition makes sense!