Separate out Package utilities & enforce better typing discipline#1502
Separate out Package utilities & enforce better typing discipline#1502isovector wants to merge 12 commits into
Conversation
|
Any reason this wasn't merged? |
|
@Ericson2314 I don't have the necessary bits, and once I realized the refactoring effort isn't likely to be fruitful, it didn't seem worth pushing on. |
|
Well if you consider this done, than I suppose @gbaz or someone can merge it if he wants accordingly. |
|
@Ericson2314 This section of the tech proposal draft suggests that "incremental refactoring is not feasible". I merged previous PRs before the tech proposal was announced, and I still thought an iterative refactoring was the plan. Now that a more comprehensive plan has been announced, I think the people responsible for the rewrite should have full access if it is to be executed, and it doesn't make sense for me to review iterative work like this, given that I am not intimately familiar with the details of the rewrite, and I won't have time to properly review it. Since I do not have access to deployments, all my previous work has been based on local testing, and then resolving remaining issues based on feedback from Gershom. I don't think that model works with the "live migration" model announced in the tech proposal. Please note that this just explains my own inaction, nothing else. I have no particular opposition to the code of this PR, and I am very grateful that Tweag is contributing. Regarding the processing of the proposal: The tech proposal repo dictates that it is HF leadership that has the final say on whether proposals are accepted or not. I don't know if HF leadership has started reviewing the proposal yet, but surely it wouldn't make sense to start executing the draft plan before it has been vetted? |
|
I can understand not bothering to finish this PR if a different approach is being gone with. However, it is both non-draft and so appears finished (?) and approved. So it may be just a matter of hitting the button. I liked the look of it / liked the changes as described in the PR description, so I made my SQL refactor on top of it. |
This PR moves the
Utilitiessection out ofDistribution.Server.Packages.Typesand intoDistribution.Server.Packages.Utils. It then adds:These new datatypes used to be anonymous tuples, and their new existence gives an easy-to-target API for the work in #1486. This PR is careful to not use the new
MetadataRevisionconstructor anywhere, so that it can be trivially swapped out for a structure that can be pulled out of the database.In addition, we keep
OldUploadInfoaround, since swapping it inside of theacid-statewould be a breaking serialization change. Furthermore, all of the utility implementations are copied verbatim intoPackages.Utils.Acid, since they're still required for*.Statemaintenance, and will diverge more dramatically in upcoming PRs.This PR also changes the type of
pkgDesc:which makes it clear that it is a function of a specific revision, rather than of the package-as-a-whole. This in turn revealed some bugs in the feed code, which was using the wrong dates. I will send a separate PR fixing those issues.