GoogleCloudPlatform / cloud-run-button

Let anyone deploy your GitHub repos to Google Cloud Run with a single click
https://cloud.run
Apache License 2.0
526 stars 91 forks source link

Allow skipping builds and pre/post build hooks - fixes #85 #146

Closed jamesward closed 4 years ago

jamesward commented 4 years ago

Tested with: --repo_url=https://github.com/jamesward/hello-uzhttp.git --git_branch=buttonify

jamesward commented 4 years ago

Oh, and whoops - for some reason my push went to GoogleCloudPlatform instead of my fork. Will investigate why to try to avoid that in the future.

ahmetb commented 4 years ago

@jamesward the issues I see with this are:

jamesward commented 4 years ago

we are overusing hooks section for overrides. maybe these two concepts will have different needs in the future. (we can split them by simply defining a type buildConfig hook)

I'm fine with a new section. How about buildconfig as a root JSON node?

how does a "build" command know what image name/tag to build as?

There is a new env var IMAGE_URL which contains the expected container image url. For tag, We've so far been tagless, but maybe we should include the git commit in the IMAGE_URL?

how does a "build" command know what to do? for example, do I just build an image onto the local docker engine and it's pushed for me ––or am I also responsible for pushing the image?

Today the push still happens in our cloudshell_open but we could provide an app.json parameter to change that. WDYT?

could we potentially also do this as something like skipBuild: true and by adding a preBuild or postBuild hook which lets the user do it there?

That'd work too. I don't have a strong preference either way. WDYT?

ahmetb commented 4 years ago

That'd work too. I don't have a strong preference either way. WDYT?

That would help us maintain fewer things in general. People can probably even use precreate hook if skipBuild: true existed (although would seem hacky).

There is a new env var IMAGE_URL which contains the expected container image url. For tag, We've so far been tagless, but maybe we should include the git commit in the IMAGE_URL?

IMO that's good enough. Tagless still works fine. Should we be passing this env to all hooks or just this one?

I'm fine with a new section. How about buildconfig as a root JSON node?

We could probably just call it "build" as everything is technically config in that file. :)

jamesward commented 4 years ago

Cool. So how about:

{
    "build": {
        "skip": true
    },
    "hooks": {
        "prebuild": {
            "commands": [
                "./my-custom-build"
            ]
        },
        "postbuild": {
            "commands": [
                "./something"
            ]
        }
    }
}

Does it feel weird that we tell it to skip the build but then still run the hooks?

Should we be passing this env to all hooks or just this one?

I actually have a postcreate that needs IMAGE_URL (right now I just use convention) so I think we just send all the env vars to all hooks.

ahmetb commented 4 years ago

Design LGTM. I think we can use this "build" flag for bunch of overrides. For example, assume there's Dockerfile, but you want to use jib, we could implement build.jib=true in the future.

jamesward commented 4 years ago

I've updated this to use the above schema.

ahmetb commented 4 years ago

Just did a quick pass. I'll probably do another but mostly LGTM.

jamesward commented 4 years ago

Thanks. Fixed all that up. Let me know how it looks now.