Generics: The Future #1820

dlom · 2022-07-14T18:06:25Z

xref: https://issues.redhat.com/browse/HIVE-1935

openshift-ci · 2022-07-14T18:06:54Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

dlom · 2022-07-14T18:21:19Z

/hold

2uasimojo

This looks awesome @dlom!

I assume it doesn't compile yet because you haven't replaced all the occurrences of Find*Condition in the codebase, right?

apis/hive/v1/generics.go

2uasimojo · 2022-07-14T18:37:24Z

apis/hive/v1/generics.go

+}
+
+type ConditionType interface {
+	AsString() string


Out of curiosity, what happens if you do one of these (mutually exclusive) alternatives?

Use the existng fmt.Stringer interface

Nix the interface and just use string() inline in FindCondition

?

I forgot about the fmt.Stringer interface... I think I'll do something like this

type ConditionType interface { fmt.Stringer }

so that ConditionType can still be used as the generic constraint on FindCondition. Alternatively, FindCondition could take Stringers instead of ConditionTypes

I think I prefer the former. The latter would mean you would have to change every condition type to a Stringer, which isn't awful, but if it's gonna be an interface type anyway, I like the explicitness of ConditionType.

That said, what about my second bullet? Then all the condition types can just be string and we don't need an interface for it.

2uasimojo · 2022-07-14T18:39:54Z

doesn't compile yet because you haven't replaced

We crossed in the mail. Looking good. Take the Draft status off once you think it'll build, so CI can take a crack at it.

openshift-ci · 2022-07-14T20:41:08Z

@dlom: PR needs rebase.

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.

2uasimojo · 2022-07-14T21:20:23Z

@dlom: PR needs rebase.

This is gonna be a nightmare to keep rebased. Basically every PR we land is gonna cause a merge conflict :(

The first use-case of this is the new FindCondition function, which replaces several trivial array search functions.

dlom · 2022-07-20T18:51:14Z

/unhold

codecov · 2022-07-20T19:24:54Z

Codecov Report

Merging #1820 (4ae1f97) into master (707d093) will decrease coverage by 0.23%.
The diff coverage is 48.59%.

@@            Coverage Diff             @@
##           master    #1820      +/-   ##
==========================================
- Coverage   41.71%   41.47%   -0.24%     
==========================================
  Files         354      355       +1     
  Lines       33142    33239      +97     
==========================================
- Hits        13824    13785      -39     
- Misses      18155    18291     +136     
  Partials     1163     1163

Impacted Files	Coverage Δ
...g/controller/clustersync/clustersync_controller.go	`75.84% <0.00%> (ø)`
...akeclusterinstall/fakeclusterinstall_controller.go	`0.00% <0.00%> (ø)`
...g/controller/machinepool/machinepool_controller.go	`47.98% <0.00%> (ø)`
pkg/controller/metrics/metrics.go	`33.14% <0.00%> (ø)`
.../openshift/hive/apis/hive/v1/clusterclaim_types.go	`33.33% <0.00%> (-66.67%)`	⬇️
...shift/hive/apis/hive/v1/clusterdeployment_types.go	`33.33% <0.00%> (-66.67%)`	⬇️
...hift/hive/apis/hive/v1/clusterdeprovision_types.go	`33.33% <0.00%> (-66.67%)`	⬇️
...ift/hive/apis/hive/v1/clusterinstall_conditions.go	`0.00% <0.00%> (ø)`
...m/openshift/hive/apis/hive/v1/clusterpool_types.go	`33.33% <0.00%> (-66.67%)`	⬇️
...nshift/hive/apis/hive/v1/clusterprovision_types.go	`33.33% <0.00%> (-66.67%)`	⬇️
... and 20 more

dlom · 2022-07-20T20:41:18Z

/retest

2uasimojo

Love this!

One nit inline; but let's do it in a FUP so you don't have to keep chasing rebases.

/lgtm

2uasimojo · 2022-07-20T23:12:21Z

pkg/controller/clusterdeployment/clusterdeployment_controller_test.go

+				// This is a hack to get Go to accept this arbitrary string as the proper type
+				dummy := hivev1.ClusterDeploymentCondition{Type: "ClusterImageSetNotFound"}
+				lc := controllerutils.FindCondition(cd.Status.Conditions, dummy.Type)


Rather make a const for it, no?

2uasimojo · 2022-07-21T06:50:34Z

/lgtm

hello?

openshift-ci · 2022-07-21T06:51:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2uasimojo, dlom

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [2uasimojo,dlom]

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

openshift-ci · 2022-07-21T08:44:32Z

@dlom: all tests passed!

Full PR test history. Your PR dashboard.

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.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2022

openshift-ci bot requested review from 2uasimojo and akhil-rane July 14, 2022 18:10

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 14, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2022

2uasimojo reviewed Jul 14, 2022

View reviewed changes

dlom marked this pull request as ready for review July 14, 2022 18:57

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2022

openshift-ci bot requested a review from abutcher July 14, 2022 18:59

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2022

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2022

dlom force-pushed the hive-1935 branch from 59a96c3 to 1e974e5 Compare July 20, 2022 18:34

Make Condition and ConditionType generic

4ae1f97

The first use-case of this is the new FindCondition function, which replaces several trivial array search functions.

dlom force-pushed the hive-1935 branch from 1e974e5 to 4ae1f97 Compare July 20, 2022 18:38

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2022

2uasimojo reviewed Jul 20, 2022

View reviewed changes

openshift-ci bot assigned 2uasimojo Jul 20, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2022

openshift-merge-robot merged commit 34a0d90 into openshift:master Jul 21, 2022

dlom mentioned this pull request Jul 25, 2022

Minor cleanup (in a test) related to generics #1829

Merged

2uasimojo mentioned this pull request Jul 26, 2022

ClusterPool Inventory #1672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generics: The Future #1820

Generics: The Future #1820

dlom commented Jul 14, 2022 •

edited

Loading

openshift-ci bot commented Jul 14, 2022

dlom commented Jul 14, 2022

2uasimojo left a comment

2uasimojo Jul 14, 2022

dlom Jul 14, 2022

2uasimojo Jul 14, 2022

2uasimojo commented Jul 14, 2022

openshift-ci bot commented Jul 14, 2022

2uasimojo commented Jul 14, 2022

dlom commented Jul 20, 2022

codecov bot commented Jul 20, 2022 •

edited

Loading

dlom commented Jul 20, 2022

2uasimojo left a comment

2uasimojo Jul 20, 2022

2uasimojo commented Jul 21, 2022

openshift-ci bot commented Jul 21, 2022

openshift-ci bot commented Jul 21, 2022

Generics: The Future #1820

Generics: The Future #1820

Conversation

dlom commented Jul 14, 2022 • edited Loading

openshift-ci bot commented Jul 14, 2022

dlom commented Jul 14, 2022

2uasimojo left a comment

Choose a reason for hiding this comment

2uasimojo Jul 14, 2022

Choose a reason for hiding this comment

dlom Jul 14, 2022

Choose a reason for hiding this comment

2uasimojo Jul 14, 2022

Choose a reason for hiding this comment

2uasimojo commented Jul 14, 2022

openshift-ci bot commented Jul 14, 2022

2uasimojo commented Jul 14, 2022

dlom commented Jul 20, 2022

codecov bot commented Jul 20, 2022 • edited Loading

Codecov Report

dlom commented Jul 20, 2022

2uasimojo left a comment

Choose a reason for hiding this comment

2uasimojo Jul 20, 2022

Choose a reason for hiding this comment

2uasimojo commented Jul 21, 2022

openshift-ci bot commented Jul 21, 2022

openshift-ci bot commented Jul 21, 2022

dlom commented Jul 14, 2022 •

edited

Loading

codecov bot commented Jul 20, 2022 •

edited

Loading