HIVE-2629: Rewrite hack/modchecker.go to be more generic #2490
Conversation
Due to limitations in the generics system implemented by go, a small hack is employed to allow us to work generically with foreign types (logical pieces of the go.mod file)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retitle HIVE-2629: Rewrite hack/modchecker.go to be more generic |
openshift-ci
bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@dlom: This pull request references HIVE-2629 which is a valid jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@dlom: This pull request references HIVE-2629 which is a valid jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
1 similar comment
|
@dlom: This pull request references HIVE-2629 which is a valid jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@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-sigs/prow repository. I understand the commands that are listed here. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2490 +/- ##
=======================================
Coverage 46.29% 46.29%
=======================================
Files 279 279
Lines 32892 32892
=======================================
Hits 15226 15226
Misses 16388 16388
Partials 1278 1278 |
|
/cc @jstuever this has changed a lot since I last showed you it, if you're interested in giving it a glance. Eric is still the main reviewer |
There was a problem hiding this comment.
This is great, @dlom. It's sad that the language ties our hands in ways that make the resulting code have to have awkward constructs, but you've worked into that admirably.
Before we merge this, I would like to see the commit message and PR summary updated to reflect all the things that have changed. You didn't just rewrite for generics; you also implemented fixers for excludes and replaces.
But on that note: I tried to get the tool to do anything with excludes and couldn't. Do you have a test case for that?
...which leads me to wonder what we would actually expect the behavior to be in that regard. Would we want to make sure that, if any exclude with a matching path is given, all the excludes for that path are identical in both files? That probably doesn't make sense given that the root file's go.sum can (and is even quite likely to) contain more versions of any given lib, and thus be subject to more exclusions of that lib.
We don't necessarily have to answer that question here; but whatever we decide, we should call it out in the code comments and the commit/PR messaging.
| MapConflictFmtStr: "WARNING: require path %s listed at multiple versions: %v | %v\n", | ||
|
|
||
| CompareNotEqualFmtStr: "XX require %s: root(%v) apis(%v)\n", | ||
| FixFn: func(file *modfile.File, toDrop string, toAdd *modfile.Require) error { |
There was a problem hiding this comment.
nit: I'm finding the var names slightly confusing here. Maybe something like:
| FixFn: func(file *modfile.File, toDrop string, toAdd *modfile.Require) error { | |
| FixFn: func(file *modfile.File, pathToReplace string, replacement *modfile.Require) error { |
I don't love using replace in these names though, as that has meaning in the context of a modfile. Thoughts?
xref: HIVE-2629
Due to limitations in the generics system implemented by go, a small hack is employed to allow us to work generically with foreign types (logical pieces of the go.mod file). Lots of documentation is written inline
/assign @2uasimojo