Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue/3110 modules v2 moduletool commands #3473

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions changelogs/unreleased/3110-modules-v2-moduletool-commands.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
description: Reviewed all moduletool commands for v2 compatibility
change-type: major
sections:
deprecation-note: "The `inmanta module list -r` command has been deprecated in favor of `inmanta project freeze`"
destination-branches:
- master
117 changes: 61 additions & 56 deletions src/inmanta/module.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
Mapping,
NewType,
Optional,
Sequence,
Set,
TextIO,
Tuple,
Expand Down Expand Up @@ -140,11 +141,15 @@ def __str__(self) -> str:
def __hash__(self) -> int:
return self._requirement.__hash__()

@property
def specs(self) -> Sequence[Tuple[str, str]]:
return self._requirement.specs

def version_spec_str(self) -> str:
"""
Returns a string representation of this module requirement's version spec. Includes only the version part.
"""
return ",".join("".join(spec) for spec in self._requirement.specs)
return ",".join("".join(spec) for spec in self.specs)

@classmethod
def parse(cls: Type[TInmantaModuleRequirement], spec: str) -> TInmantaModuleRequirement:
Expand Down Expand Up @@ -516,7 +521,7 @@ def install(self, project: "Project", module_spec: List[InmantaModuleRequirement
"Currently installed %s-%s does not match constraint %s: updating to compatible version.",
module_name,
preinstalled_version,
",".join(constraint.version_spec_str() for constraint in module_spec),
",".join(constraint.version_spec_str() for constraint in module_spec if constraint.specs),
)
try:
env.process_env.install_from_index(requirements, self.urls, allow_pre_releases=allow_pre_releases)
Expand Down Expand Up @@ -595,7 +600,7 @@ def install(self, project: "Project", module_spec: List[InmantaModuleRequirement
"Currently installed %s-%s does not match constraint %s: updating to compatible version.",
module_name,
preinstalled_version,
",".join(constraint.version_spec_str() for constraint in module_spec),
",".join(constraint.version_spec_str() for constraint in module_spec if constraint.specs),
)
return ModuleV1.update(
project, module_name, module_spec, preinstalled.path, fetch=False, install_mode=project.install_mode
Expand Down Expand Up @@ -1934,7 +1939,7 @@ def use_virtual_env(self) -> None:
"""
self.virtualenv.use_virtual_env()

def sorted_modules(self) -> list:
def sorted_modules(self) -> List["Module"]:
"""
Return a list of all modules, sorted on their name
"""
Expand Down Expand Up @@ -2291,58 +2296,6 @@ def _get_fq_mod_name_for_py_file(self, py_file: str, plugin_dir: str, mod_name:
rel_py_file = os.path.relpath(py_file, start=plugin_dir)
return loader.PluginModuleLoader.convert_relative_path_to_module(os.path.join(mod_name, loader.PLUGIN_DIR, rel_py_file))

def versions(self) -> List["Version"]:
"""
Provide a list of all versions available in the repository
"""
versions = gitprovider.get_all_tags(self._path)

def try_parse(x: str) -> "Optional[Version]":
try:
return parse_version(x)
except Exception:
return None

versions = [x for x in [try_parse(v) for v in versions] if x is not None]
versions = sorted(versions, reverse=True)

return versions

def status(self) -> None:
"""
Run a git status on this module
"""
try:
output = gitprovider.status(self._path)

files = [x.strip() for x in output.split("\n") if x != ""]

if len(files) > 0:
print(f"Module {self.name} ({self._path})")
for f in files:
print("\t%s" % f)

print()
else:
print(f"Module {self.name} ({self._path}) has no changes")
except Exception:
print("Failed to get status of module")
LOGGER.exception("Failed to get status of module %s")

def push(self) -> None:
"""
Run a git status on this module
"""
sys.stdout.write("%s (%s) " % (self.name, self._path))
sys.stdout.flush()
try:
print(gitprovider.push(self._path))
except CalledProcessError:
print("Cloud not push module %s" % self.name)
else:
print("done")
print()

def execute_command(self, cmd: str) -> None:
print("executing %s on %s in %s" % (cmd, self.name, self._path))
print("=" * 10)
Expand Down Expand Up @@ -2543,6 +2496,58 @@ def add_module_requirement_persistent(self, requirement: InmantaModuleRequiremen
# Remove requirement from module.yml file
self.remove_module_requirement_from_requires_and_write(requirement.key)

def versions(self) -> List["Version"]:
"""
Provide a list of all versions available in the repository
"""
versions = gitprovider.get_all_tags(self._path)

def try_parse(x: str) -> "Optional[Version]":
try:
return parse_version(x)
except Exception:
return None

versions = [x for x in [try_parse(v) for v in versions] if x is not None]
versions = sorted(versions, reverse=True)

return versions

def status(self) -> None:
"""
Run a git status on this module
"""
try:
output = gitprovider.status(self._path)

files = [x.strip() for x in output.split("\n") if x != ""]

if len(files) > 0:
print(f"Module {self.name} ({self._path})")
for f in files:
print("\t%s" % f)

print()
else:
print(f"Module {self.name} ({self._path}) has no changes")
except Exception:
print("Failed to get status of module")
LOGGER.exception("Failed to get status of module %s")

def push(self) -> None:
"""
Run a git push on this module
"""
sys.stdout.write("%s (%s) " % (self.name, self._path))
sys.stdout.flush()
try:
print(gitprovider.push(self._path))
except CalledProcessError:
print("Cloud not push module %s" % self.name)
else:
print("done")
print()


@stable_api
class ModuleV2(Module[ModuleV2Metadata]):
Expand Down
86 changes: 60 additions & 26 deletions src/inmanta/moduletool.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
from argparse import ArgumentParser
from collections import OrderedDict
from configparser import ConfigParser
from typing import TYPE_CHECKING, Any, Dict, List, Optional, Pattern, Set
from typing import TYPE_CHECKING, Any, Dict, List, Optional, Pattern, Sequence, Set

import texttable
import yaml
Expand Down Expand Up @@ -387,6 +387,9 @@ def modules_parser_config(cls, parser: ArgumentParser) -> None:

create = subparser.add_parser("create", help="Create a new module")
create.add_argument("name", help="The name of the module")
create.add_argument(
"--v1", dest="v1", help="Create a v1 module. By default a v2 module is created.", action="store_true"
)

freeze = subparser.add_parser("freeze", help="Set all version numbers in project.yml")
freeze.add_argument(
Expand Down Expand Up @@ -528,7 +531,20 @@ def get_modules(self, module: Optional[str] = None) -> List[Module]:
else:
return self.get_project(load=True).sorted_modules()

def create(self, name: str) -> None:
def create(self, name: str, v1: bool) -> None:
if v1:
self._create_v1(name)
else:
module_dir: str = name
if os.path.exists(module_dir):
raise Exception(f"Directory {module_dir} already exists")
cookiecutter(
"https://github.com/inmanta/inmanta-module-template.git",
checkout="v2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we perform a checkout from master? The intention is to have the V2 template on the master branch after the release, no? It looks like no v2 branch exist at the moment. Was this functionality tested?

Copy link
Contributor

Choose a reason for hiding this comment

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

I expect we will need the V1 a little longer until we have set up the proper distribution channels for v2 modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd discussed both this and #3473 (comment) with Wouter yesterday but I'd forgotten to share the conclusions. The main reason we need to keep v1 on master for now is that new v2 modules require a v2 std, which we haven't decided how to distribute yet. The v2 branch does not exist yet but this is part of inmanta/inmanta-module-template#13.

extra_context={"module_name": name},
)

def _create_v1(self, name: str) -> None:
project = self.get_project()
mod_root = project.modulepath[-1]
LOGGER.info("Creating new module %s in %s", name, mod_root)
Expand Down Expand Up @@ -568,6 +584,9 @@ def create(self, name: str) -> None:

def do(self, command: str, module: str) -> None:
for mod in self.get_modules(module):
if not isinstance(mod, ModuleV1):
LOGGER.warning("Skipping module %s: v2 modules do not support this operation.")
continue
try:
mod.execute_command(command)
except Exception as e:
Expand All @@ -578,48 +597,55 @@ def list(self, requires: bool = False) -> None:
List all modules in a table
"""
table = []
name_length = 10
version_length = 10

project = Project.get()
project.get_complete_ast()

names = sorted(project.modules.keys())
specs = project.collect_imported_requirements()
names: Sequence[str] = sorted(project.modules.keys())
specs: Dict[str, List[InmantaModuleRequirement]] = project.collect_imported_requirements()
for name in names:

name_length = max(len(name), name_length)
mod = Project.get().modules[name]
mod: Module = Project.get().modules[name]
version = str(mod.version)
if name not in specs:
specs[name] = []

try:
if project.install_mode == InstallMode.master:
reqv = "master"
else:
release_only = project.install_mode == InstallMode.release
versions = ModuleV1.get_suitable_version_for(name, specs[name], mod._path, release_only=release_only)
if versions is None:
reqv = "None"
else:
reqv = str(versions)
except Exception:
LOGGER.exception("Problem getting version for module %s" % name)
reqv = "ERROR"
generation: str = str(mod.GENERATION.name)

version_length = max(len(version), len(reqv), version_length)
reqv: str
matches: bool
editable: bool
if isinstance(mod, ModuleV1):
try:
if project.install_mode == InstallMode.master:
reqv = "master"
else:
release_only = project.install_mode == InstallMode.release
versions = ModuleV1.get_suitable_version_for(name, specs[name], mod._path, release_only=release_only)
if versions is None:
reqv = "None"
else:
reqv = str(versions)
except Exception:
LOGGER.exception("Problem getting version for module %s" % name)
reqv = "ERROR"
matches = version == reqv
editable = True
else:
reqv = ",".join(req.version_spec_str() for req in specs[name] if req.specs) or "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

The usage of reqv is inconsistent between V1 and V2 modules.

  • For V1 modules, reqv contains a list of version that match the version spec
  • For V2 modules, reqv contains the version specs.

Isn't this strange?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree somewhat strange, though both Wouter and I are of the opinion that the v2 format is an improvement. I opted to stick with the stricter approach for v1 because it needs to take the compiler version into account, making it more complex than just the version specs. We could clean that up as well but I'm not sure it's worth the effort for v1.
A small remark though, for v1 modules, if I understand correctly, it never contains a list, always a single version: the latest version with a compatible compiler version constraint.

matches = all(version in req for req in specs[name])
editable = mod.is_editable()

table.append((name, version, reqv, version == reqv))
table.append((name, generation, editable, version, reqv, matches))
Copy link
Contributor

Choose a reason for hiding this comment

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

The version_length variable is not used anywhere. We can clean that up in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


if requires:
print("requires:")
for name, version, reqv, _ in table:
LOGGER.warning("The `inmanta module list -r` command has been deprecated.")
for name, _, _, version, _, _ in table:
print(" - %s==%s" % (name, version))
else:
t = texttable.Texttable()
t.set_deco(texttable.Texttable.HEADER | texttable.Texttable.BORDER | texttable.Texttable.VLINES)
t.header(("Name", "Installed version", "Expected in project", "Matches"))
t.header(("Name", "Generation", "Editable", "Installed version", "Expected in project", "Matches"))
for row in table:
t.add_row(row)
print(t.draw())
Expand Down Expand Up @@ -736,13 +762,19 @@ def status(self, module: Optional[str] = None) -> None:
Run a git status on all modules and report
"""
for mod in self.get_modules(module):
if not isinstance(mod, ModuleV1):
LOGGER.warning("Skipping module %s: v2 modules do not support this operation.", mod.name)
continue
mod.status()

def push(self, module: Optional[str] = None) -> None:
"""
Push all modules
"""
for mod in self.get_modules(module):
if not isinstance(mod, ModuleV1):
LOGGER.warning("Skipping module %s: v2 modules do not support this operation.", mod.name)
continue
mod.push()

def verify(self) -> None:
Expand All @@ -768,6 +800,8 @@ def commit(
"""
# find module
module = self.get_module(module)
if not isinstance(module, ModuleV1):
raise CLIException(f"{module.name} is a v2 module and does not support this operation.", exitcode=1)
# get version
old_version = parse_version(str(module.version))

Expand Down