Skip to content
This repository has been archived by the owner on Oct 22, 2021. It is now read-only.

fix Dockerfile build #40

Closed

Conversation

bruce-ricard
Copy link

@bruce-ricard bruce-ricard commented Aug 24, 2020

The build of this Dockerfile started failing a little while ago.

This commit makes sure that the required tools are present before running
bin/build

We chatted with some of you on Slack last week about it.
We weren't sure if we communicated properly the change we were thinking of making, so we just made the PR so you know exactly what we meant. Please let us know if you think that's not an acceptable change.

I think that it also makes sense to add the make tools to the bin/build script itself, since you probably can't run bin/build without having run make tools, even outside of the container.

@peterhaochen47 and I

* Make sure that the required tools are present before running
bin/build

Co-Authored-by: Bruce Ricard <bricard@vmware.com>
@manno
Copy link
Member

manno commented Aug 26, 2020

So we normally prepare the build environment before running docker: https://github.com/cloudfoundry-incubator/quarks-secret/blob/master/.github/workflows/ci.yaml#L77
Getting the shared scripts into 'tools/quarks-utils' is part of the build env setup.

Second thing is, I want to get rid of make in CI and such, as it's another level of indirection. Worse, the Makefile tends to collect configuration in form of env vars. So having the Makefile as a UI for users, but have automation use the scripts directly.

We could add the script 'bin/tools' to the Dockerfile, but then our CI would call it twice. Following that logic we would soon add 'bin/tools' to all the other scripts. Then someone says, hey let's make bin/tools smarter, so it only downloads if there are changes and we start writing more of these enterprise bash scripts with color output and such... :)

So what's your use case? Are you building in Concourse?

@bruce-ricard
Copy link
Author

So we normally prepare the build environment before running docker: https://github.com/cloudfoundry-incubator/quarks-secret/blob/master/.github/workflows/ci.yaml#L77
Getting the shared scripts into 'tools/quarks-utils' is part of the build env setup.

IMHO this feels like bad practice. The Dockerfile should be buildable on its own without any environment setup required. As far as I understand, that's what dockerhub wants you to do, through its auto build feature. When you setup dockerhub's autobuild, the only thing you can do is tell it where the Dockerfile is, and it will try to build it. You won't be able to run any setup first.
On a similar note, I was personally recently trying to have a script generate a Dockerfile from some other files, but that is not OK either, because the Dockerfile wouldn't be available from github without a script running.

So what's your use case? Are you building in Concourse?

Yes. Similarly to dockerhub's autobuild, concourse has a Docker resource which gets a Dockerfile and attempts to build it before pushing it to dockerhub. And there's no way to tell it to run some setup script before that.

Second thing is, I want to get rid of make in CI and such, as it's another level of indirection. Worse, the Makefile tends to collect configuration in form of env vars. So having the Makefile as a UI for users, but have automation use the scripts directly.

That sounds interesting but I don't really understand what you mean. Could you elaborate please? I usually like indirection, it often means "abstraction", and I like abstraction :)

We could add the script 'bin/tools' to the Dockerfile, but then our CI would call it twice. Following that logic we would soon add 'bin/tools' to all the other scripts. Then someone says, hey let's make bin/tools smarter, so it only downloads if there are changes and we start writing more of these enterprise bash scripts with color output and such... :)

I don't understand that either. What's wrong with scripts that have colored output? And what is "and such" ?

@manno
Copy link
Member

manno commented Aug 27, 2020

Getting the shared scripts into 'tools/quarks-utils' is part of the build env setup.

IMHO this feels like bad practice. The Dockerfile should be buildable on its own without any environment setup required. As far as I understand, that's what dockerhub wants you to do, through its [auto build](https://docs.docker.com/docker-

We talked about that in retro and the feeling was, that it's ok to have setup. Still, in this case it's only for the version number and it would be nice to remove the dependency from the docker file. We felt like dockerhub's autobuild is only for simple projects, but quarks-secret should be a simple project...

We'll investigate.

So what's your use case? Are you building in Concourse?

Yes. Similarly to dockerhub's autobuild, concourse has a Docker resource which gets a Dockerfile and attempts to build it before pushing it to dockerhub. And there's no way to tell it to run some setup script before that.

Yes, we actually do that with a preparation step: https://github.com/cloudfoundry-incubator/quarks-ci/blob/master/pipelines/cf-operator-check/pipeline.yml#L107-L132

But it wasnt very clear to us, because the previous Concourse task was using the Makefile, which downloaded the tools.

Second thing is, I want to get rid of make in CI and such, as it's another level of indirection. Worse, the Makefile tends to collect configuration in form of env vars. So having the Makefile as a UI for users, but have automation use the scripts directly.

That sounds interesting but I don't really understand what you mean. Could you elaborate please? I usually like indirection, it often means "abstraction", and I like abstraction :)

I want less magic and a cleaner call graph. I want small, readable scripts and visible dependencies.
So, it was like this Concourse CI script -> Makefile -> bin/script -> bin/include/lib, only that it was multiple calls to make and others. Also scripts calling each other, too.

If the CI script would just prepare the environment and then call the Makefile only once, make could optimize the build graph. It's good at running dependencies only once.
But CI often needs to do things in between and as the Makefile was not specifically written for CI it has too. Aslo we don't want to do everything in make.

Then you have the scripts, which for convenience might be calling each other: bin/integration could also run 'bin/build-image', because we need the image anyways. This gets complex fast, what if you want to override the image, etc.
It often feels like that logic is not worth the maintenance/readability cost.

We tried to avoid that "conflict of interest", by splitting CI scripts from 'dev scripts', but with Travis and Github actions, the line became more muddy again (because they didn't require as much setup as Concourse and it was really easy to re-use the existing scripts).

I'd say indirection is nice, when it helps with decoupling, but not when it masks the actual tasks?

We could add the script 'bin/tools' to the Dockerfile, but then our CI would call it twice. Following that logic we would soon add 'bin/tools' to all the other scripts. Then someone says, hey let's make bin/tools smarter, so it only downloads if there are changes and we start writing more of these enterprise bash scripts with color output and such... :)

I don't understand that either. What's wrong with scripts that have colored output? And what is "and such" ?

Shell scripts are a huge pain. They are so hard to do right. Everytime I look at the output from shellcheck I'm learning something new and terrible (pushd can fail!?? Why is this not using set -euxo pipefail...).
So I'd argue for smaller scripts, with less line noise and bells and whistles. If a script needs argument parsing, color output, configurable debug modes, I'd rather have that done in another language, or replaced by a building block, like a Concourse task or Github action.

Take a look at https://github.com/cloudfoundry-incubator/quarks-ci/blob/master/pipelines/tasks/test-integration.sh
That doesn't make me happy :)
It has

  • a trace file
  • 'strict' mode
  • base dir detection, so you Concorse can run it from anywhere
  • exit trap for cleanup
  • 8 required input parameters
  • mistakes like the missing quotes in line 27
  • not sure why it relies on env to find sh

Anyhow, it does work with an existing cluster, so it's not fair to compare it, but the Github action (using KinD) is so much simpler: https://github.com/cloudfoundry-incubator/quarks-operator/blob/master/.github/workflows/ci.yaml

@bruce-ricard
Copy link
Author

OK, I completely agree about shell scripts.

About the original topic: I'd still say that you shouldn't need setup. IMHO it's like running tests. Imagine you have tests that you can run by running make test, but it doesn't work out of the box, because you haven't run scripts/configure.sh tests, and a user has no way of figuring that out from that repo. That wouldn't be great.

You maintain an OSS repo, and I tried to come use it. I tried to build your Dockerfile, but I just couldn't make it work. The only place where I could have figured out how to do it is the pipeline you just shared with me, assuming I can read concourse yaml... So I had to come to your Slack channel and ask you. If it was a closed source repo, I would say it's not ideal because a new team member in the future will waste time figuring out how it works. But for an OSS repo it really feels quite bad.

At the very least, it seems like there could be a little shell (sorry) script build_docker_image.sh:

make tools
docker build .

but honestly I would still prefer if docker build would just work from a freshly cloned repo.

@manno
Copy link
Member

manno commented Aug 28, 2020

I agree about the accessibilty, I'll add a story to investigate how we can simplify the docker build.

We just updated our docs site https://quarks.suse.dev/docs/development/ and added more instructions on the dev environment setup.

Imagine you have tests that you can run by running make test, but it doesn't work out of the box, because you haven't run scripts/configure.sh tests, and a user has no way of figuring that out from that repo. That wouldn't be great.

Every open-source project ever ...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants