Show Reconciling State in UI

[joshri]

Closes #2286

Adds code to the computeReady function to recognize an object that is currently reconciling, and display a new icon in return. As discussed in Slack, the criteria to determine if any object is reconciling is if the conditions object has
"type" == "Ready", AND
"status" == "Unknown", AND
"reason" == "Progressing"
This is the same across all objects.

This involved changing the status filter and sort functions as well.

Icon in the PageStatus component

Icon as it shows up in the data table with KubeStatusIndicator

ReconcileInTable.mov

[ozamosi]

I find nested ternary operators terrifying. I know react programmers eat ternaries for breakfast, but I used to be a PHP programmer where ternary operators are broken, so I'm very quick to start hissing when I come across them. So I looked extra carefully, and I think I have found some potential problems.

I think that this isn't doing what the cover message says - I think "type" == "Available", "status" == "Unknown", "reason" == "Progressing" would make this show up as reconciling. Is that OK? Is that intentional?

I also think that the last line of the old version turned the string "True" into true, whereas the new code returns the string "True"? Is that on purpose?

The above things look like bugs to me, so I want to talk more about them before approving. The rest of this comment is offering thoughts and ideas - not instructions.

If I wanted to remove the nested ternaries, I would reorder it to use multiple returns - so

if (_.find(conditions, {type: "Ready", status: "Unknown", reason: "Progressing"}) {
    return "Reconciling";
}
// old function goes here

Some people will say multiple returns is bad code - those people are wrong. 🙃 Sneaky multiple returns is bad code, but a clear pattern of "if statement, return, next if statement, return" is easy to follow - when I read the code I only have to care about the current if clause because nothing before or after will do anything, which makes the things I need to remember very small. Whereas right now, I'm having to keep the whole function in my head at once. It's not too bad in this case, but as I said I'm scared of nested ternary operators.

So then to make the old function follow the same pattern, I'd write it something like (maybe. I'm not sure this is better on its own, but I think it'd fit in better if the function grows with more if statements)

if (_.find(conditions, (c) => ["Ready", "Available"].includes(c.type) && c.status == "True") {
    return true;
}
return false;

And yes, these _.find( lines I've suggested are long and messy, and pulling out, like, const isReadyish = (c) => ["Ready", "Available"].includes(c.type) to use in the if conditionals would maybe be nicer.

And now that I'm in nitpick mode, "boolean or string" is a yucky return type. ReconciliationGraph is still treating this as a boolean, and I'm not saying it's wrong to - just, from looking at it, it's not clear if that is intended or not. Now, I like enums - enums error when you spell them wrong. Always using a string instead is OK, since that'd still force you to do if (computeReady(parent.conditions) != "Not Ready") and I can see it's not an accident. Another way of thinking about it is "return the valid state, if there is one" and consider "Not Ready" to not be a valid state - so, a return type of string? or enum?. I think the "True" return I complained about at the top means that you're almost there - we return a string, or false - but if that's what you're doing could you change the false to, like, null or undefined so it means "absent" rather than "untrue"?

[joshri]

I find nested ternary operators terrifying. I know react programmers eat ternaries for breakfast, but I used to be a PHP programmer where ternary operators are broken, so I'm very quick to start hissing when I come across them. So I looked extra carefully, and I think I have found some potential problems.

I think that this isn't doing what the cover message says - I think "type" == "Available", "status" == "Unknown", "reason" == "Progressing" would make this show up as reconciling. Is that OK? Is that intentional?

I also think that the last line of the old version turned the string "True" into true, whereas the new code returns the string "True"? Is that on purpose?

Yes and Yes -
1st yes - apparently some objects return "Available" instead of "Ready" but it's the same thing? Obviously I'm not a hundred percent sure but the code has always been like that and I can't see why that would change whether it's reconciling or not.
2nd yes - We can't use a boolean anymore here as there more than two potential return values. It's not just ready or not ready anymore. Suspended was different because it came from a separate place aka not in the conditions array. I'm down to change false to "False" - it's just nice to be able to use a falsy value instead of testing for the string "false"

Also - totally down to change the nested ternary garbage to ifs. It's definitely easier to read

[opudrovs]

An alternative refactoring suggestion based on @ozamosi 's comments (enums and "early bail out" pattern):

export enum ReadyType {
  Reconciling = "Reconciling",
  True = "True",
}

export function computeReady(conditions: Condition[]): ReadyType | undefined {
  if (!_.find(conditions, (c) => c.type === "Ready" || c.type === "Available"))
    return undefined;

  return _.find(conditions, (c) => c.status === "Unknown") &&
    _.find(conditions, (c) => c.reason === "Progressing")
    ? ReadyType.Reconciling
    : ReadyType.True;
}

Now sure how it should work in more complex cases like the one Robin mentioned: "type" == "Available", "status" == "Unknown", "reason" == "Progressing", so it can be modified as needed.

Other code which uses computeReady function should be updated as needed too.

[opudrovs]

I think it might be worth it to replace string values, returned from the computeReady function, with enum values.

There are four instances in the code of comparing the return value of computeReady to a hardcoded/"magic" string "Reconciling", so there are several chances to mistype something without being notified of it. When mistyping enum values we would be warned at least.

[opudrovs]

If you need space to display the "Not Ready" text you can keep it in the string value:

NotReady = "Not Ready",

[opudrovs]

Got lost in three ternaries here.

Would it be possible to

• assign the calculated the icon type to a constant before the return statement
• simplify the multiple ternaries or replace them with if/elses for readability
• just pass the final calculated value as the type prop instead of the whole complex if/else statement?

[opudrovs]

Sorry, commented on the last commit not on changed files in general, moving my comments to the changed files view. Github is quirky.

[opudrovs]

LGTM! 🎉

[opudrovs]

A constant could be used for the conditions key instead of hardcoded strings.

[joshri]

good upgrade