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

Add "dry_run" #185

Closed
wants to merge 1 commit into from
Closed

Add "dry_run" #185

wants to merge 1 commit into from

Conversation

matthope
Copy link

Continuing the work from @pickledrick - #154

@ghostsquad
Copy link
Contributor

Basically build, but don't push? I think this could be better explained/used as push: false. Thoughts?

@matthope
Copy link
Author

matthope commented Apr 1, 2018

@ghostsquad that name works as well as the other - dry_run is just a more generic/common way of looking at it, and other similar resources also refer to it as dry_run.

I'm comfortable with either naming.

@vito, do you have an opinion?

@ghostsquad
Copy link
Contributor

@matthope what resources are you referring to?

@vito
Copy link
Member

vito commented Apr 2, 2018

Thanks for the PR!

I don't think this'll really work out when running in a pipeline, though. After the put there will still be a get that attempts to fetch the digest. And the digest will be blank, which will be a really weird looking version.

This seems like something that would be better off as a task. A put with no side effects is kind of counter-intuitive. I could see a lot of utility in building images with a task anyway - a lot of people want that for being able to build + test an image before pushing it to a registry.

What's the use case for dry_run? Would a task be even better for it, I wonder? Configuring this resource involves configuring a registry and credentials for it, which is redundant if you're just gonna be setting dry_run anyway.

@ghostsquad I prefer dry_run as it calls out the use case a bit more (and defaults to false instead of having a bool that defaults to true). Having push: false kind of begs the question of why you've disabled it.

@int-tt
Copy link

int-tt commented Apr 25, 2018

I was just trying to prepare the same pull request.
As a use case, you want to run test at doker build time, and do not want to push it.
https://github.com/concourse/docker-image-resource/blob/master/Dockerfile

- put: docker-image-example
    params:
      build: repository
      dockerfile: repository/test/Dockerfile
      cache: true
      cache_tag: latest
      target: tests
      dry_run: true

@matthope
Copy link
Author

matthope commented May 4, 2018

What's the use case for dry_run? Would a task be even better for it, I wonder? Configuring this resource involves configuring a registry and credentials for it, which is redundant if you're just gonna be setting dry_run anyway.

Having a task for this could achieve the same goals, I think - it just means we'll have two different (task and resource) ways to do what is nearly exactly the same thing (currently).

Moving all this around to have separate things (tasks, resources) for building then uploading a image would address the requirement for dry_run in a fairly clean fashion.

Unfortunately, this hits on the issue where docker build requires a bit of mucking around and to be run privileged - which currently this resource makes easier to manage. kaniko may be a good alternative solution here.

@vito
Copy link
Member

vito commented May 29, 2018

@matthope Gotcha. Yeah, kaniko and similar projects have given us a lot to think about for this resource. I think the end goal for this PR is some sort of task, which for now could just steal setup code from this resource and run with privileged: true. I'm going to close this PR for now though because I don't think this change will work as part of the resource model. Thanks for submitting it and adding tests though!

@vito vito closed this May 29, 2018
@lukasmrtvy
Copy link

@matthope can you give an example building image from Dockerfile from git resource without pushing it to registry?

@ghostsquad
Copy link
Contributor

@vito please reopen this. I strongly disagree that an "appropriate solution" is to "steal setup code from this resource" and make a task.

The scenario is a simple one: I have PR, and I want to make sure the docker image can be built, but I don't need it to be pushed.

@vito
Copy link
Member

vito commented Jul 16, 2018

@ghostsquad I fully understand the scenario and its importance to many people. There is no point reopening this PR, as it isn't going to work as part of the resource interface. If there are no side effects, it cannot function as a put step, because the get will have nothing to fetch. No amount of strong disgreement is gonna change that. :)

If someone wants to submit a different PR to embed a task in this resource image (see this resource for example), reusing the existing setup code as I described, that would be a step in the right direction. It would at least, like, work.

@ghostsquad
Copy link
Contributor

All you need to make this work is instructions next to the parameter that indicate get will not work if you using dry_run, with an example.

I really hope you've seen the cfcommunity/slack-notification-resource. It doesn't even have a get or check. It's also widely popular with over 10M pulls.

image

@ghostsquad
Copy link
Contributor

An alternative to this would be to put to a local file. Build an image and run docker save afterwards. That seems valid. It also enables some other workflows that don't want to push (immediately)

@vito
Copy link
Member

vito commented Jul 18, 2018

The Slack notification resource implements a no-op check and get. It can do this because it's used for a single workflow, where the versions don't matter and there are never any bits to produce.

The docker-image resource, on the other hand, is an actual resource, with actual versioned bits, and an actual check, get, and put that are fully defined around a given workflow. To work around all that, you'd have to change or parameterize the behavior of the entire resource (i.e. in source, not in params) to guarantee the version history is consistent.

Going a step further, notification resources aren't exactly an example of good resource design. There's a reason we're planning to implement much better support for notifications in the future (see early iterations of concourse/rfcs#1). There are a ton of downsides to shoehorning notifications into the resource model: redundant check containers, a bunch of useless boxes in the pipeline UI, a ton of duplicated config to annotate them on all the jobs, a bunch of useless caches from the no-op get steps, etc. etc.

Also, put can't just write to local files - they don't make it out of the container, by design. Resources talk to an external source of truth, and the only way for a put to produce something is for its get to fetch the side effect that it performed. To write to local files, and to not have side-effects, you would use a task. That's what they're meant for.

The docker-image resource is part of the core, so it'd be better if we didn't hack around our own designs. You're free to fork, and in fact we're in need of reducing functionality in this resource and having multiple smaller resources for different workflows (see #190).

I would still suggest that the "Concoursey" outcome of this would fundamentally be a task, not a resource. If there are pain points there that are pushing you away from that approach, we should understand them and design improvements to Concourse's core.

@sjwl
Copy link

sjwl commented Jan 28, 2020

I would like to test the docker image can actually be built, without pushing it. So dry_run would serve that purpose I think.

The reason I don't want to do this inside a task is because tasks don't have access to some metadata that I want like BUILD_ID.

@RichardBradley
Copy link

What is the current best approach to build a Docker image without pushing it anywhere?
@vito, I see you referred to a task to do this in your comment above. Does such a task currently exist? Where can I find documentation on how to use this?

Thanks,

Rich

@vito
Copy link
Member

vito commented Dec 15, 2020

@RichardBradley yep! https://github.com/vito/oci-build-task

@RichardBradley
Copy link

Thanks very much.
That task is buildkit running inside a Concourse Docker, right?
So this will have noticeably different behaviour to the "docker-image-resource" with "dry_run=true", e.g. concourse/oci-build-task#9 ?
:-(

I'll give it a go some time and update here, but I might stick with my current approach of building with "docker-image-resource" and sending to an ignored ECR for the short term.

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.

8 participants