refactor: Extract payload related functions for clarity #97
Conversation
openshift-ci-robot
added
approved
smarterclayton
force-pushed
the
payload_2
branch
from
db1f5f6 to
4c7792c
Compare
| return nil | ||
| } | ||
|
|
||
| var ( |
There was a problem hiding this comment.
These will only reside in sync_worker until the graph iterator (pulling chunks of work from the payload graph and executing in parallel) is added.
smarterclayton
force-pushed
the
payload_2
branch
from
847a3eb to
65ad91d
Compare
|
@abhinavdahiya I know this looks scary, wanted to split it now (mostly) before I play with a different parallel execution |
👻 I just couldn't get to it today, was busy with some other work. I'll take a look at this tomorrow first thing |
|
Sorry to keep hitting you with these large reviews
…On Thu, Jan 24, 2019 at 8:41 PM Abhinav Dahiya ***@***.***> wrote:
@abhinavdahiya <https://github.com/abhinavdahiya> I know this looks
scary, wanted to split it now (mostly) before I play with a different
parallel execution
👻 I just couldn't get to it today, was busy with some other work. I'll
take a look at this tomorrow first thing
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#97 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p-4XxrBI1dMseTUpsLJD5s-pZE3xks5vGmDRgaJpZM4aOEIA>
.
|
|
/retest |
| errs = append(errs, errors.Wrapf(err, "error parsing %s", file.Name())) | ||
| continue | ||
| } | ||
| for _, m := range ms { |
There was a problem hiding this comment.
nit: so a bunch of manifests will have the same name... can we add that as comment to Name field like please do not use it to dedup or as a map key.
There was a problem hiding this comment.
yes, maybe it should be OriginalFilename
| if image == nil || !equalUpdate(configv1.Update{Image: image.ReleaseImage}, update) { | ||
| glog.V(4).Infof("Loading image") | ||
| // cache the payload until the release image changes | ||
| validPayload := w.payload |
There was a problem hiding this comment.
nit: this change s/image/validPayload/ seems to reverse the idea from #98
There was a problem hiding this comment.
nvm, i think it is to keep up with package name pkg/payload.
There was a problem hiding this comment.
Yeah, I did a find and replace before, but went back to being it here to have payload be the internals.
|
The refactor in d14c401 looks a good one-to-one move with no major changes to behavior. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, smarterclayton The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold for #97 (comment) |
openshift-ci-robot
added
the
do-not-merge/hold
The cvo package is getting a bit full. In preparation for parallel payload processing where we want to have a readable graph processor move payload related functions into their own package. The syncTask struct is renamed to payload.Task. Record the filename of the manifest when read and store it on Task for reference in debugging and parallelization.
smarterclayton
force-pushed
the
payload_2
branch
from
65ad91d to
00b490f
Compare
|
Updated with comment (move to OriginalFilename and add a comment). |
|
New changes are detected. LGTM label has been removed. |
|
Integration test was broken by rebase, I'll disable it until there is a working fix. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/test integration |
|
/test integration |
|
/retest |
smarterclayton
added
lgtm
4 participants
The cvo package is getting a bit full. In preparation for parallel payload processing where we want to have a readable graph processor, move payload related functions into their own package. The syncTask struct is renamed to payload.Task. Code movement, a few structs are made public purely for the ability to use diff.ObjectReflectDiff on them in testing.
Extracted from #88 to make it easier to change the applier function.