many: extract lifecycle ordering into own module #2159

kyrofa · 2018-06-07T22:02:44Z

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?

Currently the snapcraft CLI has step names and ordering hard-coded in a number of places, and those areas where it isn't hard-coded, a difficult-to-read index method is used. Simplify and extract this logic into a single module used by the others to determine lifecycle steps and ordering.

Currently the snapcraft CLI has step names and ordering hard-coded in a number of places, and those areas where it isn't hard-coded, a difficult-to-read index method is used. Simplify and extract this logic into a single module used by the others to determine lifecycle steps and ordering. Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

sergiusens · 2018-06-08T01:41:32Z

snapcraft/internal/lifecycle/constants.py

 SNAPCRAFT_INTERNAL_DIR = os.path.join('snap', '.snapcraft')
-STEPS_TO_AUTOMATICALLY_CLEAN_IF_DIRTY = {'stage', 'prime'}
+STEPS_TO_AUTOMATICALLY_CLEAN_IF_DIRTY = {steps.STAGE, steps.PRIME}


would this look better if it were an attribute of the step?

Ah, excellent idea!

sergiusens · 2018-06-08T01:42:25Z

snapcraft/internal/pluginhandler/__init__.py

+    sources,
+    states,
+    steps,
+)


this import style really drives me nuts for some reason I cannot explain 😛

Haha, this one was long enough I had to split it, sorry!

If you use autopep8 it will use the format I like :-P

sergiusens · 2018-06-08T01:48:12Z

snapcraft/internal/pluginhandler/__init__.py

-            next_step = step
-
-        return next_step
+        latest_step = None


if what we return is the next_step, should what we eval be the current_step instead of the latest_step? I am not sure why, but calling this latest_step confuses me, we had this conversation already as this is triggering things (I think we even opened a bug for this).

Ah, everything revolves around latest_step now it seems

No logic has changed here, it was just called "last step" which never felt quite right, so I changed it to "latest". Everything naturally revolves around which step was run most recently, since they build on each other. Do you prefer another name?

sergiusens · 2018-06-08T01:57:57Z

snapcraft/internal/steps.py

+
+
+class Step:
+    def __init__(self, name: str) -> None:


I am just going to throw this out, but what if instead of keeping a tight coupling with STEPS and those index calls to know where it lives we create the Step instance with knowledge of the next step and the previous step, kind of like a linked list and create a map of step names to the instances?

Yeah I thought about that, but ended up having two issues:

Order needs to be provided somehow, whether that's by providing a list giving the order or by hard-coding it into each Step instance. The former seemed simpler to understand and modify than the latter (which also seems potentially error-prone).

We need a way to know which step is first. The STEPS variable provides that knowledge (STEPS[0]).

Basically, I liked the idea of having order solidly maintained in a single, easy-to-understand place rather than spread through multiple classes and needing to know that PULL is the first step. It's an interesting idea, though. Let me try it real quick, I think it's a small change. Then we can see what we think.

Alright, just pushed it up. Let me know what you think.

We discussed offline, and I've moved it back to a list, but kept the same methods.

…fecycle_steps

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

codecov-io · 2018-06-08T18:47:41Z

Codecov Report

Merging #2159 into master will decrease coverage by 0.07%.
The diff coverage is 87.93%.

@@            Coverage Diff             @@
##           master    #2159      +/-   ##
==========================================
- Coverage   91.34%   91.26%   -0.08%     
==========================================
  Files         194      195       +1     
  Lines       12246    12317      +71     
  Branches     1820     1827       +7     
==========================================
+ Hits        11186    11241      +55     
- Misses        718      730      +12     
- Partials      342      346       +4

Impacted Files	Coverage Δ
snapcraft/internal/common.py	`97.1% <ø> (-0.03%)`	⬇️
snapcraft/internal/lifecycle/constants.py	`100% <ø> (ø)`	⬆️
snapcraft/cli/lifecycle.py	`93.75% <100%> (+0.16%)`	⬆️
snapcraft/internal/errors.py	`99.53% <100%> (+0.02%)`	⬆️
snapcraft/internal/lxd/_project.py	`92.06% <100%> (+4%)`	⬆️
snapcraft/internal/lifecycle/_runner.py	`90% <100%> (-0.16%)`	⬇️
snapcraft/internal/meta/_manifest.py	`100% <100%> (ø)`	⬆️
snapcraft/internal/states/_state.py	`88.23% <100%> (+0.23%)`	⬆️
snapcraft/internal/lifecycle/_packer.py	`85% <100%> (ø)`	⬆️
snapcraft/internal/steps.py	`72.05% <72.05%> (ø)`
... and 4 more

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 4cbe8c1...aa44ec3. Read the comment docs.

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

sergiusens · 2018-06-11T13:33:44Z

snapcraft/internal/lxd/_project.py

@@ -112,10 +112,10 @@ def clean(self, parts: List[str], step: str):
                    print('Deleting {}'.format(self._container_name))
                    subprocess.check_call([
                        'lxc', 'delete', '-f', self._container_name])
-            step = 'pull'
+            step = steps.PULL.name


I see two options here, but one thing first, we should rename the signature of this method to taking step_name instead of step or have it pass down an actual Step, where a possible value is None and move the DEPRECATED warning a level up when using strip passing it the appropriate step.

If using the former, I wouldn't get the name just to get the step again, but instead, only create the step for the case of strip

Agreed, fixed by passing down a Step.

sergiusens · 2018-06-11T13:39:45Z

snapcraft/internal/steps.py

+        return hash(self.name)
+
+    def __repr__(self):
+        return 'snapcraft.internal.steps.Step({!r})'.format(self.name)


It would of been nice to have some sort of __modpath__ to not hardcode this, closest I know of is __file__, but it will look too hacky. If we ever move this module, this change will most likely go unnoticed, so why not remove the leading qualifiers?

Yeah, you're right. I'm mostly going off of python docs here that say:

[...] If at all possible, this should look like a valid Python expression that could be used to recreate an object with the same value

So I added the qualifiers. I agree that it's just asking to break though, and I question its usefulness anyway. This is really just to make test failures nice, so we can make it whatever we want. I'll remove the leading qualifiers.

sergiusens · 2018-06-11T13:40:36Z

snapcraft/internal/steps.py

+PULL = Step('pull', False)
+BUILD = Step('build', False)
+STAGE = Step('stage', True)
+PRIME = Step('prime', True)


maybe add STRIP = PRIME with a comment about it being there for backwards compat?

That doesn't really help us since the step .name would still be prime. It's okay though, I think we can handle this a level above rather than pushing deprecation all the way down here.

sergiusens

one last round of changes. Looking good.

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

sergiusens · 2018-06-11T18:28:30Z

snapcraft/cli/lifecycle.py

-    if build_environment.is_host:
-        step = step or steps.next_step(step).name
+
+    if step:


it gets confusing a bit when step can be a step_name or an actual instance of Step.

Agreed, fixed by using an identifier decl for click to make it use a different parameter name.

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

sergiusens · 2018-06-12T02:51:21Z

\o/

sergiusens reviewed Jun 8, 2018

View reviewed changes

kyrofa added 2 commits June 8, 2018 09:54

Merge remote-tracking branch 'origin/master' into feature/refactor_li…

9cc60af

…fecycle_steps

steps: use linked list for ordering

ddf5445

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

kyrofa added 2 commits June 8, 2018 13:07

steps: move back to single list

ceed729

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

steps: add automatically clean if dirty attribute

c4e43d6

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

kyrofa mentioned this pull request Jun 8, 2018

many: automatically redo step for specified part #2152

Merged

6 tasks

sergiusens reviewed Jun 11, 2018

View reviewed changes

kyrofa added 2 commits June 11, 2018 10:55

lxd: accept Step to clean

028f652

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

steps: remove qualifiers from Step __repr__

d6ec4ea

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

sergiusens reviewed Jun 11, 2018

View reviewed changes

sergiusens approved these changes Jun 11, 2018

View reviewed changes

cli: use step_name as the option name in clean

db42639

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

sergiusens approved these changes Jun 11, 2018

View reviewed changes

Use steps.Step when handling duplicates

aa44ec3

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

kyrofa merged commit 6a676f2 into canonical:master Jun 12, 2018

kyrofa deleted the feature/refactor_lifecycle_steps branch June 12, 2018 02:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: extract lifecycle ordering into own module #2159

many: extract lifecycle ordering into own module #2159

kyrofa commented Jun 7, 2018 •

edited

sergiusens Jun 8, 2018

kyrofa Jun 8, 2018

kyrofa Jun 8, 2018

sergiusens Jun 8, 2018

kyrofa Jun 8, 2018

sergiusens Jun 11, 2018

sergiusens Jun 8, 2018

sergiusens Jun 8, 2018

kyrofa Jun 8, 2018 •

edited

sergiusens Jun 8, 2018

kyrofa Jun 8, 2018 •

edited

kyrofa Jun 8, 2018

kyrofa Jun 8, 2018

codecov-io commented Jun 8, 2018 •

edited

sergiusens Jun 11, 2018

kyrofa Jun 11, 2018

sergiusens Jun 11, 2018

kyrofa Jun 11, 2018

kyrofa Jun 11, 2018

sergiusens Jun 11, 2018

kyrofa Jun 11, 2018

sergiusens left a comment

sergiusens Jun 11, 2018

kyrofa Jun 11, 2018

sergiusens commented Jun 12, 2018

many: extract lifecycle ordering into own module #2159

many: extract lifecycle ordering into own module #2159

Conversation

kyrofa commented Jun 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Jun 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Jun 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 8, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiusens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiusens commented Jun 12, 2018

kyrofa commented Jun 7, 2018 •

edited

kyrofa Jun 8, 2018 •

edited

kyrofa Jun 8, 2018 •

edited

codecov-io commented Jun 8, 2018 •

edited