Skip to content

Conversation

calavera
Copy link
Contributor

@calavera calavera commented Apr 9, 2015

Part of #12151

Signed-off-by: David Calavera david.calavera@gmail.com

@calavera calavera mentioned this pull request Apr 9, 2015
41 tasks
@calavera calavera changed the title Fix typo in builder/dispatchers.go [WIP] Remove engine.Job from Start and Create Apr 9, 2015
@calavera calavera force-pushed the remove_job_from_start_and_create branch 4 times, most recently from 5ea2ceb to a16f48a Compare April 9, 2015 22:14
@calavera calavera changed the title [WIP] Remove engine.Job from Start and Create Remove engine.Job from Start and Create Apr 9, 2015
@icecrime
Copy link
Contributor

icecrime commented Apr 9, 2015

@calavera I think you only checked test-unit and test-integration-cli, unfortunately our old test-integration suite is still part of the campaign, and those are very broken 😉

)

if err := job.DecodeEnv(r.Body); err != nil {
if err := env.Decode(r.Body); err != nil {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Member

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

Copy link
Contributor Author

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:

  1. Get rid of all references to Job.
  2. Replace env and dynamic lookups with type safe decoding.

I rather follow that script than this other one:

  1. Get rid of some job references.
  2. Replace env and dynamic lookups with type safe decoding.
  3. Replace env references in the remaining jobs with the new type safe decoding.
  4. Get rid of remaining jobs references.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@calavera calavera force-pushed the remove_job_from_start_and_create branch 9 times, most recently from a84e23d to 77d438b Compare April 13, 2015 23:22
@calavera calavera force-pushed the remove_job_from_start_and_create branch from 77d438b to cd56797 Compare April 14, 2015 16:57
@calavera calavera force-pushed the remove_job_from_start_and_create branch from cd56797 to 0bfcdea Compare April 14, 2015 17:07
@calavera calavera force-pushed the remove_job_from_start_and_create branch from 0bfcdea to 7900e8e Compare April 14, 2015 22:30
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>
@calavera calavera force-pushed the remove_job_from_start_and_create branch from 7900e8e to 1093045 Compare April 14, 2015 22:34
@calavera calavera force-pushed the remove_job_from_start_and_create branch from 1093045 to 9e60502 Compare April 14, 2015 22:37
@calavera calavera force-pushed the remove_job_from_start_and_create branch from 9e60502 to 4bdd1d5 Compare April 14, 2015 23:36
Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera calavera force-pushed the remove_job_from_start_and_create branch from 4bdd1d5 to 767df67 Compare April 15, 2015 17:22
@calavera
Copy link
Contributor Author

OMG this is green!

@LK4D4 tons of changes to remove those pesky env references. Do you mind to double check and add some suggestions?

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 15, 2015

@calavera Thank you very much. I'll check soon.

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 16, 2015

LGTM

1 similar comment
@cpuguy83
Copy link
Member

LGTM

cpuguy83 added a commit that referenced this pull request Apr 16, 2015
@cpuguy83 cpuguy83 merged commit de923f5 into moby:master Apr 16, 2015
@calavera calavera deleted the remove_job_from_start_and_create branch April 16, 2015 03:56
@calavera
Copy link
Contributor Author

🤘

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants