Refactor migration logic for recorded app

[praveenrewar]

Here's an attempt at making the migration logic a bit more readable and simple.

I realised that I we were trying to avoid updating the createOrUpdate with the migration logic and in doing so it resulted in createOrUpdate being used only for create operations and not for updates, and this led me to make these changes.

• Created new private methods find and create.
• The above methods are then used in CreateOrUpdate, RenamePrevApp, Exists, Rename, setMeta, etc,. to make them more simple.
• Removed createOrUpdate
• Set isMigrated at relevant places.

[100mik]

Just a few nits!
Will take a closer look on my local.
Love how this makes it legible <3

[sethiyash]

Looks really good to me.

[100mik]

Changes look good!
Thanks for making this one. LGTM!

[neil-hickey]

LGTM.

This new pattern of doXANDY() felt odd, but I think it does make it more readable. I was trying to find a way to keep the flow of doX() if x { return y() } instead of the doXandY() style this introduces, but I couldn't see a nice way. I don't want to block the refactor as I think it's a step in a more readable direction, but if it's valuable I can go through and see if what I am trying to describe provides the value I am hoping it does...

foundMigratedApp, err := a.findAndUpdate(a.fqName(), labels)
if err != nil {
	return err
}

if foundMigratedApp {
	a.isMigrated = true	
	return nil
}

would love if this could remain something like: (only because I am a fan on functions doing one thing only - the pattern of AND or even OR I find confusing)

if exists, err := a.find(a.fqName()); err != nil {
    return err
}

if exists {
  a.isMigrated = true	
  return a.updateApp(labels)
}

[praveenrewar]

only because I am a fan on functions doing one thing only

Yeah, that sounds good. I will give this a shot. The only reason I didn't do it was because of the RenamePrevApp (it would become too lengthy)

[praveenrewar]

@neil-hickey Thanks a lot for your suggestions, I have made the relevant changes. Let me know your thoughts on it 😄

[neil-hickey]

@neil-hickey Thanks a lot for your suggestions, I have made the relevant changes. Let me know your thoughts on it 😄

I prefer this pattern, it's more explicit what is going on. For example:

foundMigratedApp, err := a.findAndUpdate(a.fqName(), labels)
if err != nil {
	return err
}

if foundMigratedApp {
	a.isMigrated = true	
	return nil
}

vs (now)

app, foundMigratedApp, err := a.find(a.fqName())
if err != nil {
	return err
}

if foundMigratedApp {
	a.isMigrated = true
	return a.updateApp(app, labels)
}

You can see that they are effectively the same even with the old refactor of update into the findAndUpdate function. To me, now it's really clear what's happening if the app is found.. we update it, whereas before it was kinda hidden. Curious to see what others think!

[praveenrewar]

I myself like the new changes more than previous one.

[100mik]

Still looking good to me 🚀