Define a single source of version information #1477
Conversation
gdemonet
added
topic:tests
Hello gdemonet,My role is to assist you with the merge of this Status report is not available. |
Branches have divergedThis pull request's source branch To avoid any integration risks, please re-synchronize them using one of the
Note: If you choose to rebase, you may have to ask me to rebuild |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: $ git fetch
$ git checkout -B w/2.1/improvement/640-single-source-of-image-versions origin/development/2.1
$ git merge origin/improvement/640-single-source-of-image-versions
$ # <intense conflict resolution>
$ git commit
$ git push -u origin w/2.1/improvement/640-single-source-of-image-versions |
gdemonet
force-pushed
the
improvement/640-single-source-of-image-versions
branch
from
8380de5 to
bb181f4
Compare
We define a single source of version information for the whole project, and use Python to define such a file to easily read it from other locations, such as tests or docs. This module will be used throughout the buildchain, but should never contain anything build-specific (such information can be located in the relevant parts of other buildchain modules). Issue: #640
gdemonet
force-pushed
the
improvement/640-single-source-of-image-versions
branch
4 times, most recently
from
82cd411 to
8162e23
Compare
gdemonet
force-pushed
the
improvement/640-single-source-of-image-versions
branch
3 times, most recently
from
f11d048 to
62fdd15
Compare
Previously, we pulled by digest and applied the declared version as a tag. This implied that updating the declared version only changed the tag of the image, without actually changing the image itself. We now make sure to pull the desired version with both Docker and Skopeo, checking the SHA256 digest of the result against what we specify in `buildchain.versions`. Issue: #640
Type hints were accurate w.r.t. the expected behaviour, but the actual behaviour wasn't. Note that we disable the pylint rule that complains about explicit `return None` statements.
We attempt to provide a more obvious API by using accurate names and concepts from the Docker concepts. In particular, our previous use of "registry" actually meant "repository". We also make sure that image "tags" actually represent both the name and version. Fixes: #947
We introduce a number of helper methods to generate the required targets, hence losing the declarative quality of the previous approach. However, the `versions` module provides a simpler, more readable equivalent. Issue: #640
This allows for a complete removal of the `packages/packages.list` file. Issue: #640
This will prevent the buildchain from always downloading packages if no version changed since the previous execution. The use of `doit.tools.config_changed` allows us to emulate the previous behaviour of having a `file_dep` on `packages/packages.list`. Issue: #640
We perform a simple templating of the SaltStack Yum repository definition file within the `metalk8s-build` container image. The Salt version comes from `buildchain.versions.SALT_VERSION`. Issue: #640
In order to read the list of versions defined in the buildchain from Salt formulas, we need to render them into a static file (we chose JSON, since YAML requires adding a dependency, and both are the easiest to read from). This is then read by `map.jinja` as an extension to the existing `defaults.yaml`. Issue: #640
We adjust the `pkg_installed` and `build_image_name` macros to make use of the version information we retrieved from the generated `versions.json` file. Issue: #640
When installing packages with the `pkg_installed` macro, we found that packages having declared an "Epoch" always fail to install (because we don't specify it in `versions.json`). Since we do not care about differences between epochs, we just pass the accurate flag to ignore such differences. Issue: #640
The adapted macro allows to remove all previous occurences of hardcoded versions for container images. Issue: #640
Slight refactor of the installation states for packages in
`preflight.[recommended|mandatory]`.
Note: previous usage with `{{ pkg_installed() }}` is not functional
anymore, one always need to pass a package name to the macro.
Issue: #640
We introduce a test to verify the running Kubernetes version against our list of versions from the buildchain. Note: we create a symlink, under `tests/versions.py`, pointing to `buildchain/buildchain/versions.py`, for ease of importation within tests. Issue: #640
gdemonet
force-pushed
the
improvement/640-single-source-of-image-versions
branch
from
62fdd15 to
123d1e3
Compare
The previous implementation had the following issues: - a bug when listing from a repository containing multiple versions of a single package (can happen with dirty ISO tree during development), as it was not able to choose the 'good one'. - bad naming of functions/variables and imperfect documentation, leading to misunderstandings w.r.t. the goal of this method We fix the issues by: - implement filtering based on the `repo:packages` pillar information, which is already used by the `pkg_installed` macro - renaming and documenting these custom methods for easier maintenance Fixes: #1455
This file is read from the `versions` Python module, so we need to copy it along with the test sources. Note that we squash previous `scp` occurences into a single `tar` copy over SSH, which allows us to preserve symlinks. Issue: #640
gdemonet
force-pushed
the
improvement/640-single-source-of-image-versions
branch
from
123d1e3 to
8b6ca93
Compare
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: $ git fetch
$ git checkout -B w/2.2/improvement/640-single-source-of-image-versions origin/development/2.2
$ git merge origin/w/2.1/improvement/640-single-source-of-image-versions
$ # <intense conflict resolution>
$ git commit
$ git push -u origin w/2.2/improvement/640-single-source-of-image-versions |
| 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 |
There was a problem hiding this comment.
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 returnNone¹- 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).
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following reviewers are expecting changes from the author, or must review again: |
In an attempt to clean the signature of the `docker_command` task callables, we used the `Optional[TaskError]` return type hint. However, this expressely required us to add a `return None` statement for `mypy`, and disable a `pylint` rule to do so. Instead, we use the return type hint `None` and don't `return` from such callables, leaving the `Optional[TaskError]` to the `task_error` decorator. Fixes: #1489 See: #1477
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
|
@bert-e approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue None. Goodbye gdemonet. |
slaperche-scality
deleted the
improvement/640-single-source-of-image-versions
branch
Component: build, salt, kubernetes, tests
Context:
Version information is currently hardcoded in multiple places (buildchain, Salt formulas, docs), making it difficult when some of them need to be changed.
Summary:
buildchain/buildchain/versions.pysalt/metalk8s/versions.jsoncontaining versions for packages and container imagespkg_installedandbuild_image_namemetalk8s_package_manager(see Package manager list wrong deps #1455)Acceptance criteria:
buildchain/buildchain/versions.pyfileCloses: #640
Closes: #947