cli: update parts cache in the container

[kalikiana]

• Have you followed the guidelines for contributing?
• Have you signed the CLA?
• If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
• If this is a new feature. Have you discussed the design on the forum?
• Have you successfully run ./runtests.sh static?
• Have you successfully run ./runtests.sh unit?

---

• Pass snapcraft update through to the container
  See the forum proposal.

Note: The other part is implemented in #1412.

[sergiusens]

Looks good, a question related to a test and how this would affect the behavior of define and search when the environment variable is set where it would potentially never be updated locally.

[sergiusens]

I don't think you need this fake logger setup here; or what is it intended for as there are no checks for it. self.run_command would return an object with an output and exit_code attribute.

[kalikiana]

You're right! Thanks for pointing it out! I'd copied it because it's used in other tests, but actually the output is the same - we could probably simplify a few other tests this way at some point.
I dropped the logger and added a check for exit code (not sure if it's really necessary here, but can't hurt to have it).

[kyrofa]

This looks fine, although in the proposal you say:

Proposal

• Run snapcraft update in the container
• Run apt-get update, apt-get upgrade and snap refresh as part of update - this would be similar to how the clean command without arguments deletes the container
• Stop running apt-get update with all other commands

I proposed a PR implementing the proposal.

All I see here is the first in that list, no?

[kyrofa]

When using an external template like this, named parameters are far more clear and scalable than positional ones.

[kalikiana]

I just copied it from the snap test :-D but yeah, I changed it to {snap_type}

[kalikiana]

@kyle Note that the PR description explicitly mentions the part that this PR implements. This one is update. #1412 covers the refresh/ upgrade.

[sergiusens]

There is a make_snapcraft_yaml in the parent class for tests which you can just use, right?

[kalikiana]

I basically went with the style used in other tests there, which have their own customized version of the method for repeated use. But you're right, in this case I can keep it really simple. So, done.

[kalikiana]

Tests are green again!

[elopio]

This looks good to me, but I'm wondering why do we have to update the parts in the host?

[kalikiana]

Because snapcraft define and snapcraft search operate on the host. There's little reason to start a container for those... or do you think we should?

[sergiusens]

On mar, oct 24, 2017 at 2:29 AM, Leo Arias ***@***.***> wrote: @elopio commented on this pull request. In snapcraft/cli/parts.py: > """Updates the parts listing from the cloud.""" + container_config = env.get_container_config() + if container_config.use_container: + project_options = get_project_options(**kwargs) + lifecycle.containerbuild('update', project_options, container_config) This looks good to me, but I'm wondering why do we have to update the parts in the host?

If you are outside of a project directory these commands should still work, and as such define and search should probably always run from the context of the host while update needs to happen in both places. A note of that ^ should be made in the code so it doesn't fall through the cracks in the future.

…

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[elopio]

Thanks for the comment, it makes my doubts go away.

[kyrofa]

This has my vote of approval, thanks @kalikiana.