Skip to content

Conversation

sergiusens
Copy link
Collaborator

Signed-off-by: Sergio Schvezov [email protected]

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh tests/unit?

Copy link
Contributor

@cmatsuoka cmatsuoka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using friendly names is a good idea and it's necessary because "Launching a VM" is not technically accurate for lxd. Added a pondering about adding the action to it as well, considering other future build providers, but feel free to disagree.

@@ -100,7 +100,11 @@ def _execute( # noqa: C901
else:
raise provider_error

echo.info("Launching a VM.")
echo.info(
"Launching a {}.".format(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should include the action itself in the per-provider supplied string. "Launching a VM" and "Launching a provider" sound correct, but if we have host or remote build providers, "Building on local host" could sound better than "Launching a local build". This could be supplied by e.g. get_instance_type_friendly_action() or something like that. It also could help translations if we get into this path later on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a plan, but let me come up with a better classmethod name :-)

@codecov-io
Copy link

codecov-io commented Apr 4, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@351ce36). Click here to learn what that means.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2522   +/-   ##
=========================================
  Coverage          ?   89.16%           
=========================================
  Files             ?      201           
  Lines             ?    13615           
  Branches          ?     2057           
=========================================
  Hits              ?    12140           
  Misses            ?     1039           
  Partials          ?      436
Impacted Files Coverage Δ
snapcraft/internal/build_providers/_lxd/_lxd.py 24.2% <0%> (ø)
snapcraft/cli/lifecycle.py 85% <0%> (ø)
.../internal/build_providers/_multipass/_multipass.py 83.76% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 351ce36...708ecab. Read the comment docs.

Copy link
Contributor

@cmatsuoka cmatsuoka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a simpler solution that seems to do everything we need!

@sergiusens sergiusens merged commit e8efe52 into canonical:master Apr 5, 2019
@sergiusens sergiusens deleted the instance-name branch April 5, 2019 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants