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

Build docker image based on aiida-prerequisites #3722

Merged
merged 10 commits into from
Jan 25, 2020
Merged

Build docker image based on aiida-prerequisites #3722

merged 10 commits into from
Jan 25, 2020

Conversation

yakutovicha
Copy link
Contributor

@yakutovicha yakutovicha commented Jan 22, 2020

fixes #3725

@ltalirz
Copy link
Member

ltalirz commented Jan 22, 2020

For the record, this is an implementation of the discussion on the plugin testing infrastructure from the coding week (document now frozen).

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @yakutovicha !

one comment + please add a job to build the docker file here

.docker/opt/configure-aiida.sh Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Some minor comments

.docker/opt/configure-aiida.sh Outdated Show resolved Hide resolved
.docker/opt/configure-aiida.sh Outdated Show resolved Hide resolved
.docker/opt/configure-aiida.sh Outdated Show resolved Hide resolved
.docker/opt/configure-aiida.sh Outdated Show resolved Hide resolved
@yakutovicha
Copy link
Contributor Author

yakutovicha commented Jan 23, 2020

I will now work on adding things to github workflows. @ltalirz and @sphuber please look at the changes in the meanwhile and see if you agree

@yakutovicha
Copy link
Contributor Author

Btw, I think it totally makes sense to move the whole profile setup to quicksetup. What do you guys think?

@yakutovicha
Copy link
Contributor Author

@sphuber can you help me in setting up Git Hub actions? I am not sure how to do things right. The manual suggests making a new action file. Is it the way to go?

@sphuber
Copy link
Contributor

sphuber commented Jan 23, 2020

@sphuber can you help me in setting up Git Hub actions? I am not sure how to do things right. The manual suggests making a new action file. Is it the way to go?

What is it that needs to be run and when? Every commit? Take a look at the .github/workflows/ci.yml. There you will find the main job defined with the various steps that are run on each commit and PR. Depending on what you need to run, you can add one there.

@ltalirz
Copy link
Member

ltalirz commented Jan 23, 2020

What is it that needs to be run and when? Every commit?

The build of the docker image should be run on every commit

@yakutovicha
Copy link
Contributor Author

What is it that needs to be run and when? Every commit?

It should be docker build, yes, it should be run every commit.

Take a look at the .github/workflows/ci.yml. There you will find the main job defined with the various steps that are run on each commit and PR.

I did it, and the check that I've added have failed. https://github.com/aiidateam/aiida-core/pull/3722/checks?check_run_id=405089491

@sphuber
Copy link
Contributor

sphuber commented Jan 23, 2020

I did it, and the check that I've added have failed. https://github.com/aiidateam/aiida-core/pull/3722/checks?check_run_id=405089491

##[error]Process completed with exit code 100.
Run apt-get install -y --no-install-recommends docker.io
E: Could not open lock file /var/lib/dpkg/lock-frontend - open (13: Permission denied)
E: Unable to acquire the dpkg frontend lock (/var/lib/dpkg/lock-frontend), are you root?

You are not installing with sudo, so try to prepend your install statements with sudo. Also I noticed that in the actual docker build step you do not specify the location of the Dockerfile. Is it going to find it automatically?

@yakutovicha
Copy link
Contributor Author

btw, another option would be to add Docker hub status check. That requires no setup here. What do you guys think?

@yakutovicha
Copy link
Contributor Author

You are not installing with sudo, so try to prepend your install statements with sudo.

Let's try. I wasn't sure if it is allowed to use sudo here.

Also I noticed that in the actual docker build step you do not specify the location of the Dockerfile. Is it going to find it automatically?

By default, it takes it from the current folder. I hope the current folder here is the package's root.

@ltalirz
Copy link
Member

ltalirz commented Jan 23, 2020

btw, another option would be to add Docker hub status check. That requires no setup here. What do you guys think?

I think that is a good idea. We should not require it to pass, since I believe this can take some time. But it's good to take burden away from github actions, and the build on dockerhub will be needed anyhow.

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @yakutovicha !

Looks all good to me - the requests are just typos etc.

.docker/opt/configure-aiida.sh Outdated Show resolved Hide resolved
.docker/opt/configure-aiida.sh Outdated Show resolved Hide resolved
.docker/opt/configure-aiida.sh Outdated Show resolved Hide resolved
.docker/opt/configure-aiida.sh Outdated Show resolved Hide resolved
.docker/opt/configure-aiida.sh Outdated Show resolved Hide resolved
.docker/opt/configure-aiida.sh Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@yakutovicha yakutovicha marked this pull request as ready for review January 23, 2020 19:38
@yakutovicha
Copy link
Contributor Author

I think it's ready. @ltalirz or @sphuber please review.

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks for the updates @yakutovicha !

I notice two issues with the actions build:

  1. It seems like there is no profile configured, although by your default there should be
  2. The docker build did not fail despite no profile being configured. I guess you should replace the verdi profile list by a verdy profile show default

@yakutovicha
Copy link
Contributor Author

thanks for the updates @yakutovicha !

I notice two issues with the actions build:

1. It seems like [there is no profile configured](https://github.com/aiidateam/aiida-core/pull/3722/checks?check_run_id=405794933#step:5:6), although by your default there should be

2. The docker build did not fail despite no profile being configured. I guess you should replace the `verdi profile list` by a `verdy profile show default `

I think I know why let me try to fix that.

@yakutovicha
Copy link
Contributor Author

Should be fixed with that: aiidateam/aiida-prerequisites#6. Let's see...

* Use `wait-for-services` script to wait till all scripts in /etc/my_init.d executed.
* Replace `verdi profile list` with `verdi provile show default`
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
yakutovicha and others added 2 commits January 24, 2020 14:20
Co-Authored-By: Leopold Talirz <leopold.talirz@gmail.com>
Co-Authored-By: Leopold Talirz <leopold.talirz@gmail.com>
@ltalirz ltalirz self-requested a review January 24, 2020 14:01
ltalirz
ltalirz previously approved these changes Jan 24, 2020
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @yakutovicha ! From my side this is good to go.

@sphuber : Somehow it looks to me like every actions check ran twice (not jenkins)
Screenshot 2020-01-24 at 15 02 15

Not sure whether this is expected - anyhow I don't see how this PR should be responsible for it.

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @yakutovicha almost there: just two small fixes and a suggestion

.docker/opt/configure-aiida.sh Outdated Show resolved Hide resolved
.docker/opt/configure-aiida.sh Outdated Show resolved Hide resolved
.docker/opt/configure-aiida.sh Outdated Show resolved Hide resolved
@sphuber
Copy link
Contributor

sphuber commented Jan 24, 2020

Oh and one final question: why is the Dockerfile on the top level? Since there is a .docker folder, doesn't it make more sense to put it in there? Or is that technically not possible or against convention?

@ltalirz
Copy link
Member

ltalirz commented Jan 24, 2020

Oh and one final question: why is the Dockerfile on the top level? Since there is a .docker folder, doesn't it make more sense to put it in there? Or is that technically not possible or against convention?

It is sort of a convention in that this is the place where many external tools/services will look for the Dockerfile by default (mybinder, heroku, Dockerhub, etc.).
It is possible to move it to the .docker folder but I think it's not a bad idea to have this one at the top level.

@yakutovicha
Copy link
Contributor Author

@sphuber if you want to merge the PR, please let me know 5 mins in advance: I will set up automated builds on Docker Hub.

@sphuber
Copy link
Contributor

sphuber commented Jan 24, 2020

OK this is good to go, let me know when you have setup Docker Hub and I will merge

@yakutovicha
Copy link
Contributor Author

yakutovicha commented Jan 24, 2020

OK this is good to go, let me know when you have setup Docker Hub and I will merge

ready, please merge. Docker build should now be triggered automatically at Docker Hub: https://hub.docker.com/repository/docker/aiidateam/aiida-core

@sphuber sphuber merged commit 3848649 into aiidateam:develop Jan 25, 2020
@sphuber
Copy link
Contributor

sphuber commented Jan 25, 2020

Thanks @yakutovicha

@yakutovicha yakutovicha deleted the add_docker_file branch January 27, 2020 10:07
@yakutovicha yakutovicha mentioned this pull request Jan 27, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker container: make user credentials configurable
3 participants