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

Define a single source of version information #1477

Merged
Merged
Show file tree
Hide file tree
Changes from 3 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
11 changes: 6 additions & 5 deletions .pylintrc
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,12 @@ confidence=
enable=all

# Disable the message, report, category or checker with the given ID(s).
disable=abstract-method, # Too many false positive :/
bad-whitespace, # Alignment is a good thing.
bad-continuation, # I kindly disagree :)
locally-disabled, # Sometime we need to disable a warning locally.
suppressed-message # Don't pollute the output with useless info.
disable=abstract-method, # Too many false positive :/
bad-whitespace, # Alignment is a good thing.
bad-continuation, # I kindly disagree :)
locally-disabled, # Sometime we need to disable a warning locally.
suppressed-message, # Don't pollute the output with useless info.
useless-return # Disagreement with mypy, see https://github.com/python/mypy/issues/3974
gdemonet marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Going back to that, I still think it's not needed.

I think we may have messed up our type annotations.

Seeing the examples on this issue python/mypy#3157, one can see that the decorated function must be annotated with what it really returns (so if our function returns nothing, the return type must be None), because the decorator can have its own signature.

So in our case:

  • __call__ always return None¹
  • the decorator returns a function that return Optional[TaskError]

With the following diff, both pylint and mypy are happy and we don't need to disable anything.

diff --git a/.pylintrc b/.pylintrc
index ce7a5299..c4aae53d 100644
--- a/.pylintrc
+++ b/.pylintrc
@@ -47,7 +47,6 @@ disable=abstract-method,     # Too many false positive :/
         bad-continuation,    # I kindly disagree :)
         locally-disabled,    # Sometime we need to disable a warning locally.
         suppressed-message,  # Don't pollute the output with useless info.
-        useless-return       # Disagreement with mypy, see https://github.com/python/mypy/issues/3974
 
 # }}}
 # Basic {{{
diff --git a/buildchain/buildchain/docker_command.py b/buildchain/buildchain/docker_command.py
index 56e53c71..4cf26444 100644
--- a/buildchain/buildchain/docker_command.py
+++ b/buildchain/buildchain/docker_command.py
@@ -140,7 +140,7 @@ class DockerBuild:
 
     @task_error(docker.errors.BuildError, handler=build_error_handler)
     @task_error(docker.errors.APIError)
-    def __call__(self) -> Optional[TaskError]:
+    def __call__(self) -> None:
         DOCKER_CLIENT.images.build(
             tag=self.tag,
             path=self.path,
@@ -148,7 +148,6 @@ class DockerBuild:
             buildargs=self.buildargs,
             forcerm=True,
         )
-        return None
 
 
 class DockerRun:
@@ -266,14 +265,13 @@ class DockerRun:
     @task_error(docker.errors.ContainerError, handler=container_error_handler)
     @task_error(docker.errors.ImageNotFound)
     @task_error(docker.errors.APIError)
-    def __call__(self) -> Optional[TaskError]:
+    def __call__(self) -> None:
         run_config = self.expand_config()
         DOCKER_CLIENT.containers.run(
             image=self.builder.tag,
             command=self.command,
             **run_config
         )
-        return None
 
 
 class DockerTag:
@@ -294,10 +292,9 @@ class DockerTag:
 
     @task_error(docker.errors.BuildError, handler=build_error_handler)
     @task_error(docker.errors.APIError)
-    def __call__(self) -> Optional[TaskError]:
+    def __call__(self) -> None:
         to_tag = DOCKER_CLIENT.images.get(self.full_name)
         to_tag.tag(self.repository, tag=self.version)
-        return None
 
 
 class DockerPull:
@@ -338,7 +335,6 @@ class DockerPull:
                     s=self, observed_digest=pulled.id
                 )
             )
-
         return None
 
 
@@ -358,12 +354,10 @@ class DockerSave:
 
     @task_error(docker.errors.APIError)
     @task_error(OSError)
-    def __call__(self) -> Optional[TaskError]:
+    def __call__(self) -> None:
         to_save = DOCKER_CLIENT.images.get(self.tag)
         image_stream = to_save.save(named=True)
 
         with self.save_path.open('wb') as image_file:
             for chunk in image_stream:
                 image_file.write(chunk)
-
-        return None

¹: except the one you modified to check the digest that explicitly returns a task error (we may want to raise an exception instead and use task_error for that, to be consistent).


# }}}
# Basic {{{
Expand Down
39 changes: 1 addition & 38 deletions buildchain/buildchain/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,44 +46,7 @@
VAGRANT_SSH_KEY_PAIR : Path = VAGRANT_ROOT/'preshared_key_for_k8s_nodes'

# }}}
# Versions {{{

K8S_VERSION : str = '1.11.10'
SALT_VERSION : str = '2018.3.4'
KEEPALIVED_VERSION : str = '1.3.5-8.el7_6'
KEEPALIVED_BUILD_ID : int = 1

CENTOS_BASE_IMAGE : str = 'docker.io/centos'
CENTOS_BASE_IMAGE_SHA256 : str = \
'6ae4cddb2b37f889afd576a17a5286b311dcbf10a904409670827f6f9b50065e'

def load_version_information() -> None:
"""Load version information from `VERSION`."""
to_update = {
'VERSION_MAJOR', 'VERSION_MINOR', 'VERSION_PATCH', 'VERSION_SUFFIX'
}
with VERSION_FILE.open('r', encoding='utf-8') as fp:
for line in fp:
name, _, value = line.strip().partition('=')
# Don't overwrite random variables by trusting an external file.
var = name.strip()
if var in to_update:
globals()[var] = value.strip()


VERSION_FILE : Path= ROOT/'VERSION'

# Metalk8s version.
# (Those declarations are not mandatory, but they help pylint and mypy).
VERSION_MAJOR : str
VERSION_MINOR : str
VERSION_PATCH : str
VERSION_SUFFIX : str

load_version_information()

SHORT_VERSION : str = '{}.{}'.format(VERSION_MAJOR, VERSION_MINOR)
VERSION : str = '{}.{}{}'.format(SHORT_VERSION, VERSION_PATCH, VERSION_SUFFIX)
# Git project information {{{


def git_ref() -> Optional[str]:
Expand Down
33 changes: 26 additions & 7 deletions buildchain/buildchain/docker_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,14 @@ def __init__(
@task_error(docker.errors.BuildError, handler=build_error_handler)
@task_error(docker.errors.APIError)
def __call__(self) -> Optional[TaskError]:
return DOCKER_CLIENT.images.build(
DOCKER_CLIENT.images.build(
gdemonet marked this conversation as resolved.
Show resolved Hide resolved
tag=self.tag,
path=self.path,
dockerfile=self.dockerfile,
buildargs=self.buildargs,
forcerm=True,
)
return None


class DockerRun:
Expand Down Expand Up @@ -267,11 +268,12 @@ def expand_config(self) -> Dict[str, Any]:
@task_error(docker.errors.APIError)
def __call__(self) -> Optional[TaskError]:
run_config = self.expand_config()
return DOCKER_CLIENT.containers.run(
DOCKER_CLIENT.containers.run(
image=self.builder.tag,
command=self.command,
**run_config
)
return None


class DockerTag:
Expand All @@ -294,27 +296,42 @@ def __init__(self, repository: str, full_name: str, version: str):
@task_error(docker.errors.APIError)
def __call__(self) -> Optional[TaskError]:
to_tag = DOCKER_CLIENT.images.get(self.full_name)
return to_tag.tag(self.repository, tag=self.version)
to_tag.tag(self.repository, tag=self.version)
return None


class DockerPull:
# The call method is not counted as a public method
# pylint: disable=too-few-public-methods
"""A class to expose the `docker pull` command through the API client."""

def __init__(self, repository: str, digest: str):
def __init__(
self, repository: str, tag: str, digest: str
):
"""Initialize a `docker pull` callable object.
Arguments:
repository: the repository to pull from
digest: the digest to pull from the repository
tag: the image tag to pull from the repository
digest: the expected digest of the image to pull
"""
self.repository = repository
self.tag = tag
self.digest = digest

@task_error(docker.errors.BuildError, handler=build_error_handler)
@task_error(docker.errors.APIError)
def __call__(self) -> Optional[TaskError]:
return DOCKER_CLIENT.images.pull(self.repository, tag=self.digest)
pulled = DOCKER_CLIENT.images.pull(self.repository, tag=self.tag)

if pulled.id != self.digest:
return TaskError(
"Image {} pulled from {} doesn't match expected digest: "
"expected {}, got {}".format(
self.tag, self.repository, self.digest, pulled.id
)
)

return None


class DockerSave:
Expand All @@ -336,7 +353,9 @@ def __init__(self, tag: str, save_path: Path):
def __call__(self) -> Optional[TaskError]:
to_save = DOCKER_CLIENT.images.get(self.tag)
image_stream = to_save.save(named=True)

with self.save_path.open('wb') as image_file:
for chunk in image_stream:
image_file.write(chunk)
return True

return None
17 changes: 9 additions & 8 deletions buildchain/buildchain/iso.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
from buildchain import targets as helper
from buildchain import types
from buildchain import utils
from buildchain import versions


ISO_FILE : Path = config.BUILD_ROOT/'{}.iso'.format(config.PROJECT_NAME.lower())
Expand Down Expand Up @@ -148,8 +149,8 @@ def task__iso_render_bootstrap() -> types.TaskDict:
return helper.TemplateFile(
source=constants.ROOT/'scripts'/'bootstrap.sh.in',
destination=constants.ISO_ROOT/'bootstrap.sh',
context={'VERSION': constants.VERSION},
file_dep=[constants.VERSION_FILE],
context={'VERSION': versions.VERSION},
file_dep=[versions.VERSION_FILE],
task_dep=['_iso_mkdir_root'],
).task

Expand All @@ -158,11 +159,11 @@ def task__iso_generate_product_txt() -> types.TaskDict:
"""Generate the product.txt file."""
def action(targets: Sequence[str]) -> None:
datefmt = "%Y-%m-%dT%H:%M:%SZ"
dev_release = '1' if constants.VERSION_SUFFIX == '-dev' else '0'
dev_release = '1' if versions.VERSION_SUFFIX == '-dev' else '0'
info = (
('NAME', config.PROJECT_NAME),
('VERSION', constants.VERSION),
('SHORT_VERSION', constants.SHORT_VERSION),
('VERSION', versions.VERSION),
('SHORT_VERSION', versions.SHORT_VERSION),
('GIT', constants.GIT_REF or ''),
('DEVELOPMENT_RELEASE', dev_release),
('BUILD_TIMESTAMP', dt.datetime.utcnow().strftime(datefmt)),
Expand All @@ -177,7 +178,7 @@ def action(targets: Sequence[str]) -> None:
'title': lambda task: utils.title_with_target1('GENERATE', task),
'actions': [action],
'targets': [constants.ISO_ROOT/'product.txt'],
'file_dep': [constants.VERSION_FILE],
'file_dep': [versions.VERSION_FILE],
'task_dep': ['_iso_mkdir_root'],
'uptodate': [False], # False because we include the build timestamp.
'clean': True,
Expand All @@ -194,7 +195,7 @@ def task__iso_build() -> types.TaskDict:
'-joliet',
'-joliet-long',
'-full-iso9660-filenames',
'-volid', '{} {}'.format(config.PROJECT_NAME, constants.VERSION),
'-volid', '{} {}'.format(config.PROJECT_NAME, versions.VERSION),
'--iso-level', '3',
'-gid', '0',
'-uid', '0',
Expand All @@ -207,7 +208,7 @@ def task__iso_build() -> types.TaskDict:
)
# Every file used for the ISO is a dependency.
depends = list(coreutils.ls_files_rec(constants.ISO_ROOT))
depends.append(constants.VERSION_FILE)
depends.append(versions.VERSION_FILE)
return {
'title': lambda task: utils.title_with_target1('MKISOFS', task),
'doc': doc,
Expand Down
20 changes: 10 additions & 10 deletions buildchain/buildchain/salt_tree.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,8 @@ def _run(self) -> None:
task_name='top.sls',
source=constants.ROOT/'pillar'/'top.sls.in',
destination=constants.ISO_ROOT/'pillar'/'top.sls',
context={'VERSION': constants.VERSION},
file_dep=[constants.VERSION_FILE],
context={'VERSION': versions.VERSION},
file_dep=[versions.VERSION_FILE],
),
)

Expand All @@ -175,8 +175,8 @@ def _run(self) -> None:
task_name='top.sls',
source=constants.ROOT/'salt'/'top.sls.in',
destination=constants.ISO_ROOT/'salt'/'top.sls',
context={'VERSION': constants.VERSION},
file_dep=[constants.VERSION_FILE],
context={'VERSION': versions.VERSION},
file_dep=[versions.VERSION_FILE],
),

Path('salt/metalk8s/addons/monitoring/alertmanager/deployed.sls'),
Expand Down Expand Up @@ -207,8 +207,8 @@ def _run(self) -> None:
'salt', 'metalk8s', 'addons', 'ui', 'files',
'metalk8s-ui-deployment.yaml'
),
context={'VERSION': constants.VERSION},
file_dep=[constants.VERSION_FILE],
context={'VERSION': versions.VERSION},
file_dep=[versions.VERSION_FILE],
),
Path('salt/metalk8s/addons/ui/precheck.sls'),

Expand Down Expand Up @@ -312,8 +312,8 @@ def _run(self) -> None:
'salt', 'metalk8s', 'kubernetes', 'mark-control-plane',
'deployed.sls'
),
context={'VERSION': constants.VERSION},
file_dep=[constants.VERSION_FILE],
context={'VERSION': versions.VERSION},
file_dep=[versions.VERSION_FILE],
),

Path('salt/metalk8s/kubernetes/sa/advertised.sls'),
Expand Down Expand Up @@ -440,10 +440,10 @@ def _run(self) -> None:
root=constants.ISO_IMAGE_ROOT,
server_root='${}_{}_images'.format(
config.PROJECT_NAME.lower(),
constants.VERSION.replace('.', '_').replace('-', '_')
versions.VERSION.replace('.', '_').replace('-', '_')
),
name_prefix='{}-{}/'.format(
config.PROJECT_NAME.lower(), constants.VERSION
config.PROJECT_NAME.lower(), versions.VERSION
),
destination=Path(
constants.ISO_ROOT,
Expand Down
Loading