Code review comment for lp:~pwlars/uci-engine/snappy-imageimport

Revision history for this message
Paul Larson (pwlars) wrote :

> Paul,
>
> Thanks for working on this, the CLI does what is expected.
>
> I am unsure about its location as an separated module. At least, we should
> s/snappy/ubuntucore/, but I don't like much the idea of creating a new
> module/directory for each CLI we planned, it is unnecessarily confusing and
> hard to find.
Yes, I couldn't think of a better place to put it, but it's not just going to be the CLI. There's another task to create a worker that uses it, so I thought it might be more appropriate to give it it's own directory as we've done with every other service so far. Putting all of these related to snappy under a single directory would be fine I think too, but don't you think we should still logically separate them somehow since they are separate microservices?

Why should it be ubuntucore instead of snappy? I'm not 100% positive of this, but it seems that "snappy" is the new term for what we are testing, thought 99% of it was once called ubuntu core.

> It needs ci_utils, so if we are going to fork/subprocess to it, we need `run-
> python` and the scripts could live in an agnostic common path, let's say bin/
> or ubuntucore/bin/, because for now we deploy the whole uci-engine tree on
> nodes.
I figured I'd let the worker just import it and sort all that out. I'm open to suggestions, but I don't think we have any intention of running this from the command line in production. For testing purposes though, it's very easy to setup a venv, install ci-utils, and run this.

> If we intend to import and use publish_image() inside workers, it should be a
> module (as the other ci-airline modules) and all CLIs could live in a module,
> let's say 'ubuntucore', which will eventually provide worker implementations
> for the services we need.
Why wouldn't we put the cli/modules with the individual service rather than all together? Are you saying they should all live together, or just all under one root like I was talking about above?

review: Needs Information

« Back to merge proposal