console-shared: add getCreationTimestamp() selector

[mareklibra]

Required by: #2600

[mareklibra]

/assign @christianvogt

[mareklibra]

/retest

[christianvogt]

/lgtm
/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, mareklibra

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• frontend/OWNERS [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[spadgett]

/hold
for #2817

[spadgett]

/hold cancel

[spadgett]

/hold

These selectors are an anti-pattern imo. We should be trying to remove them instead of adding more.

[christianvogt]

@spadgett could you explain why? They simply look like a way to use lodash get in a typed manner.

[spadgett]

Sorry, I commented on a different PR since there are several of these open.

One of my concerns is that they're not actually type safe. There's nothing guarding against having a typo in the string passed to get. In my opinion, this would be much better as

return obj ? obj.metadata.creationTimestamp : null;

metadata should not be null for any k8s resource. If it is, _.get might be masking a larger problem. If I type createdTimestamp instead of creationTimestamp, I have some hope of the TypeScript compiler catching it. It won't using get.

I don't think this scales very well. We seem to be adding these not just for the common fields, but for all properties on all types.

While not the case in this PR, I've also seen some places where we're adding helpers like these instead of defining a type for a specific resource. We get more value in the end from defining the type.

I'm not sure how this is really better than just using obj.metadata.creationTimestamp inline. The helper adds a level of indirection and weakens type checking without adding much value. It's not that difficult to add a null check for obj if needed, and in many contexts, the null check is not needed.

I also dislike that we're adding these in separate PRs instead of the PR where they're being used. That way we don't know the context during review and we might be creating orphaned code if the other PR changes or doesn't merge.

[spadgett]

Admittedly, my point is undercut by our types having [key: string]: any (fixed for metadata in #2848).

I'll remove the hold since this seems to be the established pattern. We might want to go back and implement these without _.get for better type safety. I'm just not really a fan.

/hold cancel

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-ci-robot]

@mareklibra: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-console-olm	1bbd4bc	link	/test e2e-aws-console-olm

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[mareklibra]

I also dislike that we're adding these in separate PRs instead of the PR where they're being used. That way we don't know the context during review and we might be creating orphaned code if the other PR changes or doesn't merge.

I agree. The only reason for splitting the code was to simplify/speed-up the review process.

I'm not sure how this is really better than just using obj.metadata.creationTimestamp inline. The helper adds a level of indirection and weakens type checking without adding much value. It's not that difficult to add a null check for obj if needed, and in many contexts, the null check is not needed.

I agree that having properly typed input objects and accessing fields inline is definitely better than hiding behind _.get(), so it make sense to change the selectors in this manner. Nevertheless, it is implementation detail for the selectors.

On the higher level, the actual use of selectors brings benefits over inlined-access to data within their actual use. No matter of API documentation, the received input might not conform the reality and the app should stay reliable. And these checks for deeper objects can be complex.

I believe the original argument for using _.get() over inlining access within selectors was that the result if anything is missing on the path is still the same. But I agree this might be result of a typo which typescript would help to find same as first testing run by a developer.

[spadgett]

The only reason for splitting the code was to simplify/speed-up the review process.

It makes review much harder because the reviewer doesn't know how something is being used or if it's necessary. This is particularly true when you have PRs depending on 2-3 other PRs and the branches are frozen.

Nevertheless, it is implementation detail for the selectors.

I don't see it as an implementation detail. It seems to be the whole purpose of these helpers.

No matter of API documentation, the received input might not conform the reality and the app should stay reliable.

The API documentation is generated from the code, and the API server enforces this. If that's broken, I'd honestly rather have it blow up to alert us there's a problem.

I believe the original argument for using _.get() over inlining access within selectors was that the result if anything is missing on the path is still the same.

I'd really like to get us away from _.get as it's not type safe.