-
Notifications
You must be signed in to change notification settings - Fork 18.8k
Remove engine.Job from Start and Create #12253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove engine.Job from Start and Create #12253
Conversation
5ea2ceb
to
a16f48a
Compare
@calavera I think you only checked |
) | ||
|
||
if err := job.DecodeEnv(r.Body); err != nil { | ||
if err := env.Decode(r.Body); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omg, can we replace it with types? Because it is even worse than jobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should decode the body into a typed structure. I'd rather not doing it here to keep the changes to the minimum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I propose to make it first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think making the env typed should block this PR. I think it should be a separated discussion. It touches more areas than just these two commands and we should be careful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was unclear in the issue that removing jobs involved getting ride of everything in /engine including env. Env is the worst part of the framework and actually the primary goal of the removal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the primary purpose of this to get rid of env in the first place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that the purpose of all this is to get rid of everything under /engine. I feel like it's better to do it in phases, that's what I though was the idea/plan:
- Get rid of all references to Job.
- Replace env and dynamic lookups with type safe decoding.
I rather follow that script than this other one:
- Get rid of some job references.
- Replace env and dynamic lookups with type safe decoding.
- Replace env references in the remaining jobs with the new type safe decoding.
- Get rid of remaining jobs references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@calavera I don't know where you seeing second script.
Current state of PR makes no sense. It makes nothing better, you replaced job with env, which will be deleted anyway and have same problems and even worse. It is possible to replace job with typed structs and your PR became smaller than now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LK4D4 I didn't replace anything, job calls env internally, I removed the layer on top of the env. It's not worse, we're removing the references to job, that's good.
Replacing the job with typed structs requires to go through all the project's code base to remove the reference to env before merging this. That's the second script.
a84e23d
to
77d438b
Compare
77d438b
to
cd56797
Compare
cd56797
to
0bfcdea
Compare
0bfcdea
to
7900e8e
Compare
Signed-off-by: David Calavera <david.calavera@gmail.com>
Signed-off-by: David Calavera <david.calavera@gmail.com>
Signed-off-by: David Calavera <david.calavera@gmail.com>
7900e8e
to
1093045
Compare
1093045
to
9e60502
Compare
9e60502
to
4bdd1d5
Compare
Signed-off-by: David Calavera <david.calavera@gmail.com>
4bdd1d5
to
767df67
Compare
OMG this is green! @LK4D4 tons of changes to remove those pesky env references. Do you mind to double check and add some suggestions? |
@calavera Thank you very much. I'll check soon. |
LGTM |
1 similar comment
LGTM |
Remove engine.Job from Start and Create
🤘 |
Part of #12151
Signed-off-by: David Calavera david.calavera@gmail.com