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

Create a module for building yagna Docker images #325

Merged
merged 19 commits into from
Nov 5, 2020

Conversation

kmazurek
Copy link
Contributor

@kmazurek kmazurek commented Nov 2, 2020

Resolves: #317

Summary of changes:

  • created module goth/runner/container/build.py which handles building the Docker image for yagna
    • this module uses a temporary directory to create build context for docker build
    • setting up the build context includes either downloading or copying local assets required for the build (yagna binaries, runtime .deb package)
    • building the image is triggered from goth/runner/container/compose.py#start_network
    • docker/yagna-goth.Dockerfile and docker/docker-compose.yml have been updated to reflect the updated build process
  • both artifact and release download scripts have been refactored and moved into the goth subtree under runner/download
    • downloaders are now classes which share common logic (e.g. authenticated session init, caching) via inheritance
    • downloaders now have a caching mechanism, storing artifacts and releases downloaded from GitHub in a temporary directory, identified by their GitHub IDs
    • the download scripts themselves have been moved to scripts directory under the project root
  • bumped Python version to 3.8 (required to use dirs_exist_ok parameter on shutil.copytree)
  • _run_command function from runner/container/compose.py has been extracted to a separate module under runner/process.py

@kmazurek kmazurek self-assigned this Nov 2, 2020
@kmazurek kmazurek mentioned this pull request Nov 2, 2020
6 tasks
@kmazurek kmazurek changed the title Integrate asset downloaders with test runner Create a module for building yagna Docker images Nov 4, 2020
@kmazurek kmazurek marked this pull request as ready for review November 4, 2020 09:37
class YagnaBuildEnvironment:
"""Configuration for the Docker build process of a yagna image."""

archive_path: Optional[Path]
Copy link
Contributor

Choose a reason for hiding this comment

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

@zakaprov are all of these properties independent? ... my common sense tells me you'd only ever need just one of them? ...

Copy link
Contributor

@azawlocki azawlocki Nov 5, 2020

Choose a reason for hiding this comment

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

I agree: archive_path and binary_dir could me made a single property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can merge those into a single parameter which will handle all three cases (i.e. directory, single binary and archive).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 8fc8f8d



@dataclass(frozen=True)
class YagnaBuildEnvironment:
Copy link
Contributor

Choose a reason for hiding this comment

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

given that all the functions in this file use "environment" as an argument - wouldn't it rather make sense for them all to be methods of the YagnaBuildEnvironment class?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO in this case it's mainly a matter of style and personal preferences. I like it this (i.e. non-OO) way. Plus, it makes it more clear that YagnaBuildEnvironment is immutable (for what it's worth).

context_deb_dir.mkdir()

if env.binary_dir:
logger.info("using local yagna binaries. path=%s", env.binary_dir)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have any convention for this, but just for the record: I prefer log messages starting with a capital letter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that you mentioned this I noticed that most of the INFO logs that we have start with capitals. Fine with me, I'll update these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 39f256a

Copy link
Contributor

@azawlocki azawlocki left a comment

Choose a reason for hiding this comment

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

Left some comments, mostly nitpicking except perhaps for the suggestion to replace several properties of YagnaBuildEnvironment with a single property.

Copy link
Contributor

@azawlocki azawlocki left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@shadeofblue shadeofblue left a comment

Choose a reason for hiding this comment

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

looks good as far as I can tell :)

@kmazurek kmazurek merged commit a7dc584 into master Nov 5, 2020
@kmazurek kmazurek deleted the km/downloader-refactoring branch November 5, 2020 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make downloading artifacts a part of runner
3 participants