state: allow parametrization of the global state file #2186

sergiusens · 2018-07-31T02:38:55Z

Refactor so that the global state file is not hard coded but part
of the project instead. This allows for future relocations.

LP: #1782783

Signed-off-by: Sergio Schvezov sergio.schvezov@canonical.com

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?

codecov-io · 2018-07-31T12:53:01Z

Codecov Report

Merging #2186 into master will increase coverage by <.01%.
The diff coverage is 93.65%.

@@            Coverage Diff             @@
##           master    #2186      +/-   ##
==========================================
+ Coverage   91.23%   91.24%   +<.01%     
==========================================
  Files         202      201       -1     
  Lines       12865    12899      +34     
  Branches     1907     1912       +5     
==========================================
+ Hits        11738    11770      +32     
  Misses        767      767              
- Partials      360      362       +2

Impacted Files	Coverage Δ
snapcraft/internal/states/__init__.py	`100% <ø> (ø)`	⬆️
snapcraft/internal/states/_state.py	`87.5% <ø> (-0.74%)`	⬇️
snapcraft/internal/states/_global_state.py	`100% <100%> (+18.18%)`	⬆️
snapcraft/internal/meta/_manifest.py	`100% <100%> (ø)`	⬆️
snapcraft/internal/meta/_snap_packaging.py	`90.77% <100%> (+0.02%)`	⬆️
snapcraft/internal/lifecycle/_clean.py	`85.08% <100%> (-0.13%)`	⬇️
snapcraft/internal/lifecycle/_runner.py	`92.09% <100%> (+0.04%)`	⬆️
snapcraft/project/_project_options.py	`87.87% <33.33%> (-1.36%)`	⬇️
snapcraft/project/_project.py	`85.71% <71.42%> (-14.29%)`	⬇️

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 49cf505...76d7ec9. Read the comment docs.

sergiusens · 2018-07-31T15:45:28Z

snapcraft/project/_project_options.py

@@ -231,6 +231,10 @@ def __init__(
        self._set_machine(target_deb_arch)
        self.__debug = debug

+        # These paths maintain backwards compatibility.
+        self.internal_dir = os.path.join(self.__project_dir, "snap", ".snapcraft")


maybe these two need to be _ to indicate they should not be used from plugins

Agreed. Also note that _project_options is being phased out, do you want this in Project instead?

yeah, I can move it to project

kyrofa · 2018-07-31T18:03:36Z

snapcraft/internal/states/_global_state.py

@@ -31,6 +34,24 @@ class GlobalState(State):

    yaml_tag = u"!GlobalState"

-    def __init__(self, build_packages, build_snaps):
+    @classmethod
+    def load(cls, *, filepath: str):


Missing a return type check, here.

kyrofa · 2018-07-31T18:38:03Z

snapcraft/internal/states/_global_state.py

+    def get_build_snaps(self) -> List[str]:
+        return self.assets.get("build-snaps", [])
+
+    def __init__(self, *, build_packages: List[str], build_snaps: List[str]) -> None:


Wait... this should totally be broken right now. This should be trying to pass assets as a kwarg, not build_packages or build_snaps. What's happening here?

(snapcraft) ubuntu@snapcraft-xenial-dev:~/source/make-hello$ ipython Python 3.5.2 (default, Nov 23 2017, 16:37:01) Type 'copyright', 'credits' or 'license' for more information IPython 6.4.0 -- An enhanced Interactive Python. Type '?' for help. In [1]: from snapcraft.internal.states import GlobalState In [2]: s = GlobalState.load(filepath="snap/.snapcraft/state") In [3]: s Out[3]: GlobalState(assets: OrderedDict([('build-packages', ['gcc']), ('build-snaps', ['go'])])) In [4]: cat snap/.snapcraft/state !GlobalState assets: build-packages: [gcc] build-snaps: [go]

Refactor so that the global state file is not hard coded but part of the project instead. This allows for future relocations. LP: #1782783 Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

kyrofa · 2018-08-01T20:05:06Z

snapcraft/internal/states/_global_state.py

-def _global_state_constructor(loader, node):
-    parameters = loader.construct_mapping(node)
-    return GlobalState(**parameters)
+GlobalStateT = TypeVar("GlobalStateT", bound="GlobalState")


Why this as opposed to just quoting the type within the class?

not sure 🤷‍♂️, reverted

kyrofa · 2018-08-01T20:09:16Z

snapcraft/internal/states/_global_state.py


+        self.add_constructor(u"!GlobalState", type(self).construct_global_state)


Yuck, they've made this awkward 😛 .

it could be added after class declaration too

kyrofa

Looks good, thank you!

sergiusens force-pushed the global-state-dir branch 2 times, most recently from fa6f40c to 8f37e90 Compare July 31, 2018 02:41

sergiusens commented Jul 31, 2018

View reviewed changes

kyrofa reviewed Jul 31, 2018

View reviewed changes

sergiusens added 2 commits July 31, 2018 17:51

state: allow parametrization of the global state file

e95175f

Refactor so that the global state file is not hard coded but part of the project instead. This allows for future relocations. LP: #1782783 Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

fix loading of GlobalState and add support for appending

2c0b730

Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

sergiusens force-pushed the global-state-dir branch from d4e4efb to 2c0b730 Compare July 31, 2018 20:51

sergiusens added 6 commits July 31, 2018 19:24

static

2dfeff6

typing

141e49d

fixup and add tests

3b5645b

more internal dir and global state file to Project

5262732

reuse existing state

db40bbf

ensure constructor is used

28d0dcf

kyrofa reviewed Aug 1, 2018

View reviewed changes

kyrofa approved these changes Aug 1, 2018

View reviewed changes

typing using strings

76d7ec9

sergiusens merged commit b412242 into canonical:master Aug 2, 2018

sergiusens deleted the global-state-dir branch August 2, 2018 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

state: allow parametrization of the global state file #2186

state: allow parametrization of the global state file #2186

sergiusens commented Jul 31, 2018 •

edited

codecov-io commented Jul 31, 2018 •

edited

sergiusens Jul 31, 2018

kyrofa Jul 31, 2018 •

edited

sergiusens Jul 31, 2018

kyrofa Jul 31, 2018

kyrofa Jul 31, 2018

sergiusens Jul 31, 2018

kyrofa Aug 1, 2018

sergiusens Aug 1, 2018

kyrofa Aug 1, 2018

sergiusens Aug 1, 2018

kyrofa left a comment


		self.add_constructor(u"!GlobalState", type(self).construct_global_state)

state: allow parametrization of the global state file #2186

state: allow parametrization of the global state file #2186

Conversation

sergiusens commented Jul 31, 2018 • edited

codecov-io commented Jul 31, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

kyrofa Jul 31, 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

Choose a reason for hiding this comment

kyrofa left a comment

Choose a reason for hiding this comment

sergiusens commented Jul 31, 2018 •

edited

codecov-io commented Jul 31, 2018 •

edited

kyrofa Jul 31, 2018 •

edited