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

Reformat with docker tutorial #2773

Conversation

Projects
None yet
4 participants
@oleksandr-shabelnyk
Copy link
Contributor

commented Jun 10, 2019

@markus2330 Please review my another tutorial. I thought it was a good idea to merge it with my previous tutorial, but even it is in a separate branch, I still see all the other commits... I hope it is OK.

Basics

  • [x ] Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see Coding Guidelines)
  • meta data is updated (e.g. README.md of plugins and METADATA.ini)

Review

Reviewers will usually check the following:

Labels

  • Add the "work in progress" label if you do not want the PR to be reviewed yet.
  • Add the "ready to merge" label if the build server is happy and also you
    say that everything is ready to be merged.

@markus2330 markus2330 requested a review from sanssecours Jun 10, 2019

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Seems like you branched off incorrectly. Both branches should branch off from master, not from each other.

Please add more links between the tutorials.

@oleksandr-shabelnyk

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

No, they are branched off from master, but I merged another branch first into master and then created this branch from master.... Is it very bad now?

@sanssecours
Copy link
Member

left a comment

Thank you for the pull request. I only found some minor issues. Please feel free to ignore my suggestions, if you do not like them.

@@ -0,0 +1,133 @@
# Introduction

Running all the tests like the build server requires to have multiple dependencies installed. To overcome this problem, instead of trying to install all the necessary dependencies on your own, an appropriate Docker image can be used. This way you can easily and quickly run all the tests.

This comment has been minimized.

Copy link
@sanssecours

sanssecours Jun 11, 2019

Member
Suggested change
Running all the tests like the build server requires to have multiple dependencies installed. To overcome this problem, instead of trying to install all the necessary dependencies on your own, an appropriate Docker image can be used. This way you can easily and quickly run all the tests.
Running all the tests like the build server requires multiple dependencies. To overcome this problem, instead of trying to install all the necessary dependencies on your own, an appropriate Docker image can be used. This way you can easily and quickly run all the tests.
@@ -536,6 +536,8 @@ mounted, use `kdb gen -F <plugin>:<file> elektra <parentKey> <outputName>` to lo

- For beginners we added a [tutorial](../tutorials/contributing-clion.md) that guides them through the process of contributing to libelektra. _(Thomas Bretterbauer)_
- Added a section on `elektraPluginGetGlobalKeySet` in the plugin tutorial. _(Vid Leskovar)_
- Added a step-by-step [tutorial](../tutorials/run_all_tests_with_docker.md) for running all tests with Docker. _(Oleksandr Shabelnyk)_

This comment has been minimized.

Copy link
@sanssecours

sanssecours Jun 11, 2019

Member

Thank you very much for using relative links 💗.


Running all the tests like the build server requires to have multiple dependencies installed. To overcome this problem, instead of trying to install all the necessary dependencies on your own, an appropriate Docker image can be used. This way you can easily and quickly run all the tests.

## Who is this guide for?

This comment has been minimized.

Copy link
@sanssecours

sanssecours Jun 11, 2019

Member
Suggested change
## Who is this guide for?
## Who Is This Guide For?

Please use title case for headings as described here.


## Prerequisites

- Docker for Linux containers has to be preinstalled. Please refer to https://docs.docker.com/install/ if you haven't installed it yet. Your host OS can be either Linux or Windows of course.

This comment has been minimized.

Copy link
@sanssecours

sanssecours Jun 11, 2019

Member
Suggested change
- Docker for Linux containers has to be preinstalled. Please refer to https://docs.docker.com/install/ if you haven't installed it yet. Your host OS can be either Linux or Windows of course.
- Docker for Linux containers has to be preinstalled. Please refer to https://docs.docker.com/install/ if you haven't installed it yet. Your host OS can be either Linux, macOS or Windows of course.
- Docker for Linux containers has to be preinstalled. Please refer to https://docs.docker.com/install/ if you haven't installed it yet. Your host OS can be either Linux or Windows of course.
- Basic knowledge of Docker (not mandatory)

## What to begin with?

This comment has been minimized.

Copy link
@sanssecours

sanssecours Jun 11, 2019

Member
Suggested change
## What to begin with?
## What to Begin With?
```

The building process depends on your Internet speed connection and the overall performance of your hardware. Most likely, it will take at least
5 minutes. Please be patient. Once you have it built, you will reuse it.

This comment has been minimized.

Copy link
@sanssecours

sanssecours Jun 11, 2019

Member
Suggested change
5 minutes. Please be patient. Once you have it built, you will reuse it.
5 minutes. Please be patient. Once you have built the image, you can reuse it multiple times.
The building process depends on your Internet speed connection and the overall performance of your hardware. Most likely, it will take at least
5 minutes. Please be patient. Once you have it built, you will reuse it.

The image tag `buildelektra-sid` we suggested can be of course replaced to your own preference.

This comment has been minimized.

Copy link
@sanssecours

sanssecours Jun 11, 2019

Member
Suggested change
The image tag `buildelektra-sid` we suggested can be of course replaced to your own preference.
The image tag `buildelektra-sid` we suggested can be replaced by a name of your own choosing.

The image tag `buildelektra-sid` we suggested can be of course replaced to your own preference.

### 2. Run Docker container

This comment has been minimized.

Copy link
@sanssecours

sanssecours Jun 11, 2019

Member
Suggested change
### 2. Run Docker container
### 2. Run the Docker Container

### 2. Run Docker container

Now when you built the image, spin-up a container like this:

This comment has been minimized.

Copy link
@sanssecours

sanssecours Jun 11, 2019

Member
Suggested change
Now when you built the image, spin-up a container like this:
After you built the image, you can execute a container like this:

Again, if you changed the image tag `buildelektra-sid`, please change it above as well.

### 3. Running the script

This comment has been minimized.

Copy link
@sanssecours

sanssecours Jun 11, 2019

Member
Suggested change
### 3. Running the script
### Running the Script
@alexsaber

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@sanssecours thank you very much for your comments!
I made a new commit with your changes. The PR for another tutorial (running test) resides here #2768
I am sorry for the confusion. I will apply your changes there. Please approve another PR as well.

@markus2330 markus2330 merged commit b231881 into ElektraInitiative:master Jun 12, 2019

9 of 12 checks passed

LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
bsd FreeBSD:freebsd-11-2-release-amd64 Task Summary
Details
bsd FreeBSD:freebsd-12-0-release-amd64 Task Summary
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
linux Task Summary
Details
mac Task Summary
Details
mac ASAN_OPTIONS:detect_leaks=1 BINDINGS:cpp ENABLE_ASAN:ON TOOLS:kdb Task Summary
Details
mac BUILD_FULL:ON BUILD_SHARED:OFF Task Summary
Details
mac KDB_DB_FILE:default.mmap KDB_DB_INIT:elektra.mmap KDB_DEFAULT_STORAGE:mmapstorage Task Summary
Details
@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.