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
527 stars 92 forks source link

Both Cloud Run Button and Heroku Button - app.json Clash #112

Open SudharakaP opened 4 years ago

SudharakaP commented 4 years ago

I am trying to add the Cloud Run Button to one of our repositories which has the Heroku Deploy Button which also uses an app.json file.

This creates problems when trying to add the Cloud Run button to the same repository as the Cloud Run also assumes an app.json file and the Heroku app.json file which is different in format will crash the Cloud Run when trying to deploy. Also I am unable to define an app.json file for Cloud Run since one already exists for Heroku.

Is there a possibility to configure the name of the app.json file (or just name it differently) for example so that it doesn't clash with other cloud providers such as Heroku? :smile:

ahmetb commented 4 years ago

If you can mention the error, we can help it not clash (by perhaps introducing a placeholder for fields that aren't supported).

jamesward commented 4 years ago

Yeah, our app.json parsing is strict so likely we just need more ignore flags. I think this is the file that is failing: https://github.com/SudharakaP/jhipster-registry/blob/add-gcp-cloud-run-deploy-button/app.json

I can investigate.

SudharakaP commented 4 years ago

@ahmetb : Thanks for the quick reply. The first error is;

Error: error attempting to read the app.json from the cloned repository: failed to parse app.json file: failed to parse app.json: json: cannot unmarshal string into Go struct field env.required of type bool

Now this is caused by the following line of app.yaml. When you remove the quotes around true it passes through that but gives the following error;

Error: error attempting to read the app.json from the cloned repository: failed to parse app.json file: failed to parse app.json: json: unknown field "buildpacks"

due to the buildpacks argument in the app.yaml file

Hope this helps. Let me know if you need any further details. 😄

ahmetb commented 4 years ago

We can try and fix buildpacks argument, but I assume this is not a good idea. Because that value is critical to how the app is built. And we better not ignore it.

About required being a string in Heroku ecosystem –that's really sad, and I am not sure if we can fix it easily. We'd need to write a custom json parser and hook that into the normal json parser. (This is not trivial, kubernetes has IntOrBool type for this, maybe we can learn from it).

A cleaner solution I can think of is to introduce a new file name that's used during the lookup stage. For example, we'd suggest people to still use app.json, but if there's a cloudrun.json, we'd use that first.

Maybe this hack would be just for repos like yours, and we would not make it as prominent as app.json in the documentation.

jamesward commented 4 years ago

Officially, required is a boolean in app.json https://devcenter.heroku.com/articles/app-json-schema#env

Our parsing should be able to handle that.

For the buildpacks param, I think it'd be ok to ignore it for now, with a warning. And in cases where the auto-detection / defaults work, people can then use the button. E.g. if the button doesn't work because of our lack of support, then people should add the button to their repo.

ahmetb commented 4 years ago

So required should be a boolean everywhere, in that case we shouldn't try to handle string for sure.

@SudharakaP are you able to deploy and run after deleting buildpacks field, and making required field a boolean as spec says?

jamesward commented 4 years ago

I'm guessing is actually not going to work because it is using a non-default buildpack. Maybe @jkutner can let us know if the https://github.com/jhipster/jhipster-registry-buildpack buildpack will work with the CNB heroku/buildpacks image.

SudharakaP commented 4 years ago

@ahmetb : After changing required to boolean and deleting the buildpacks field and trying to run; I get the following error;


sudharakapalamakumbura@cloudshell:~$ cloudshell_open --repo_url "https://github.com/SudharakaP/jhipster-registry.git" --git_branch "add-gcp-cloud-run-deploy-button"
[ ✓ ] Cloned git repository https://github.com/SudharakaP/jhipster-registry.git.
[ ? ] Value of JHIPSTER_PASSWORD environment variable (Admin password for the registry (used to login after clicking 'View App'). Must be at least 5 characters.) jhipster
[ ? ] Value of JAVA_OPTS environment variable (Java runtime options.) -Xmx300m -Xss512k -XX:CICompilerCount=2 -XX:ReservedCodeCacheSize=50m -XX:MaxMetaspaceSize=80m -XX:ParallelGCThreads=3 -Dfile.encoding=UTF-8
[ ? ] Value of SPRING_OPTS environment variable (Spring Boot options.) --server.undertow.io-threads=1 --server.undertow.eager-filter-init=false
[ ? ] Value of JHIPSTER_REGISTRY_VERSION environment variable (Version of the registry to deploy.) 5.0.2
[ ✓ ] Queried list of your GCP projects
[ ? ] Would you like to use existing GCP project jhipstergaemicro to deploy this app? Yes
[ ✓ ] Enabled Cloud Run API on project jhipstergaemicro.
[ ? ] Choose a region to deploy this application: us-central1
[ ! ] Attempting to build this application with its Dockerfile...
[ ! ] FYI, running the following command:
        docker build -t gcr.io/jhipstergaemicro/JHipster Registry .
[ ✖ ] Failed to build container image.
Error: this application doesn't have a Dockerfile; attempted to build it via heroku/buildpacks and failed: docker build failed: exit status 125, output:
invalid argument "gcr.io/jhipstergaemicro/JHipster Registry" for "-t, --tag" flag: invalid reference format: repository name must be lowercase
See 'docker build --help'.

sudharakapalamakumbura@cloudshell:~$
jamesward commented 4 years ago

This is because you need the custom buildpack to build this project.

SudharakaP commented 4 years ago

@jamesward : So if I understand correctly, app.json in cloud run currently doesn't support custom buildpacks? Trying to understand if there's a way we could get around this one. :thinking:

ahmetb commented 4 years ago

The string JHipster comes from app.json > name field (Or repo name), no?

jamesward commented 4 years ago

@SudharakaP Correct, Cloud Run Button doesn't support custom buildpacks defined by app.json yet. @ahmetb The name is unrelated. There is a custom buildpack defined that is needed to build the jhipster stuff.

I think a workaround for this would be to use a Dockerfile for the build. But we might still run into app.json parsing issues, which we should fix.

SudharakaP commented 4 years ago

@ahmetb : Actually I think you are right; I changed the name field to all lowercase and also changed the PORT to point to the PORT environment variable and this makes it deploy successfully. However it prints out he following error in the log when trying to authenticate.

2019-11-25 21:16:52.914 PST Container Sandbox Limitation: Unsupported syscall setsockopt(0x24,0x1,0xa,0x3e88bb822150,0x4,0x0). Please, refer to https://gvisor.dev/c/linux/amd64/setsockopt for more information.

2019-11-25 21:16:52.927 PST POST 403 434 B16 msChrome 78 https://jhipster-registry-gv3mz4r7ta-uc.a.run.app/api/authenticate

I think this is because of a limitation of the container sandbox. I am trying to see if I could do the same thing on Anthos (where there's no container sandbox). Is the Cloud Run button supports deploying on Anthos as well?

@jamesward : Thanks much for confirming; I am new to Cloud Run and learning as I go along. :smile:

ahmetb commented 4 years ago

The “syscall not supported” log is actually an info/warning. It does not mean your app is failing. Is your app actually throwing an exception? If not, it means it’s probably working fine.

SudharakaP commented 4 years ago

@ahmetb : Thanks for the information. I have to investigate more; since the authentication seems not to work; probably due to the removal of buildpacks. Anyways to summarize; it would be helpful if;

1) The buildpack argument is supported (https://github.com/GoogleCloudPlatform/cloud-run-button/issues/3)

2) Maybe have a cloudrun.json as suggested above so that we could have two different configurations for Heroku and GCP.

3) Not a huge problem, but I also have a third issue I noticed; when I click on the Cloud Run button it connects to cloud shell and executes the command,

 cloudshell_open --repo_url "https://github.com/SudharakaP/jhipster-registry.git" --page "editor" --git_branch "add-gcp-cloud-run-deploy-button"

This fails with Error: flag provided but not defined: -page. So normally each time I have to rerun the command without the -page flag;

cloudshell_open --repo_url "https://github.com/SudharakaP/jhipster-registry.git" --git_branch "add-gcp-cloud-run-deploy-button"
jkutner commented 4 years ago

https://github.com/jhipster/jhipster-registry-buildpack is not a Cloud Native Buildpack, and Heroku's app.json only supports v2 (non-CNB) buildpacks right now.

jamesward commented 4 years ago

@jkutner Thanks!

There is an existing bug about invalid container image names based on app.json: https://github.com/GoogleCloudPlatform/cloud-run-button/issues/57

jamesward commented 4 years ago

Oh, and the error about the page param is something we are fixing right now: #114

jkutner commented 4 years ago

I think we fix this tactically by adding CNB support to https://github.com/jhipster/jhipster-registry-buildpack. I've created https://github.com/jhipster/jhipster-registry-buildpack/issues/3 and will try to get a PR today.

jamesward commented 4 years ago

Thanks @jkutner! So, in the CNB world, what is the right way to handle custom specified buildpacks?

jkutner commented 4 years ago

@jamesward it will depend on the platform, but there's no specification yet (we're working on one).

Today pack allows the --buildpack option, but it's up to each platform to do it's own thing for now. I think reading app.json buildpacks is ok, but you will run into cases where it contains a v2 buildpack.

SudharakaP commented 4 years ago

There's one other problem I have; it seems that from my testing that for Cloud Run we need to change the docker files EXPOSE value to ${PORT} since GCP seems to use this environment variable (https://cloud.google.com/run/docs/reference/container-contract). This is also a problem when you have both the Heroku button and the Google Cloud Run button since Heroku doesn't have a problem with a hard-coded port value. Is there some workaround to keep the PORT value the same in the docker file and still be able to say forward that to ${PORT} ? :thinking:

jamesward commented 4 years ago

Thanks @jkutner, doesn't the --buildpack option just override the detect phase and need to be a buildpack location in the builder image or a tar/tgz? In which case we won't be able to go from the usual app.json buildpack settings to something that works with pack.

@SudharakaP With both Cloud Run and Heroku the app just needs to listen on $PORT and it doesn't matter what is in the Dockerfile's EXPOSE statement.

jkutner commented 4 years ago

@jamesward yes, the buildpack either needs to be in the builder or provided to pack as a tgz. But there's no mechanism (yet) for pack to automatically turn a Github URL into a tgz, so it would be up to the platform (i.e. Cloud Run) to convert that Github URL into something pack can use (probably by downloading it, maybe with an API key to prevent 429s, and stripping the root dir).

We've considered adding this as a first class thing in pack: https://github.com/buildpack/rfcs/pull/26

But we're waiting to implement a Buildpack Registry first.

juliemar commented 4 years ago

I have the same problem and the workaround for me was creating an exclusive branch for Google Buton de deploy. you can see my solution here

In my case, in the app.json I have to declare "stack": "container", for Heroku, and this tag is not supported for google cloud run.

my app.json file

{
  "name": "Matrix",
  "description": "Matrix produces a virtual office for remote teams. In this project, you can run a virtual office to simulate the physical environment",
  "repository": "https://github.com/ResultadosDigitais/matrix",
  "keywords": ["online workspace", "virtual office", "remote teams"],
  "stack": "container",
  "env": {
    "GOOGLE_CLIENT_ID": {
      "description": "Google Client Id to the OAuth authentication",
      "value": "xxxxxxx.apps.googleusercontent.com"
    },
    "GOOGLE_SECRET": {
      "description": "Google Secret to the OAuth authentication",
      "value": "gXXXXXXXXXXXXXXss"
    },
    "GOOGLE_CALLBACK_URL": {
      "description": "The callback Url to the OAuth authentication",
      "value": "http://localhost:8080/auth/google/callback"
    },
    "COOKIE_SESSION_SECRET": {
      "description": "The secret of session",
      "value": "my-matrix-session"
    },
    "COOKIE_SESSION_MAX_AGE": {
      "description": "The Session duration",
      "value": "2592000000"
    },
    "WHITELIST_DOMAINS": {
      "description": "The list of valid email domains to enter in matrix",
      "value": "[]"
    },
    "ENFORCE_SSL": {
      "description": "If you running in HTTPS this variable forces redirect to HTTPS when user access with HTTP",
      "value": "true"
    }
  }
}

Here is possible to see the error.

image

my suggestions

  1. Ignore not supported tags
  2. Like my workaround where I defining a specific branch, maybe we can declare a specific *.json file
jamesward commented 4 years ago

I think our current plan is to add ignored fields as they arise. I'll create a PR for that.

SpEcHiDe commented 4 years ago

Hi, I think I am facing a similar issue. I tried deleting some of the fields, but still I am getting failed to parse app.json: json: unknown field "formation" This is my repository: https://github.com/SpEcHiDe/PublicLeech/blob/master/app.json

jamesward commented 4 years ago

@SpEcHiDe Thanks for reporting! Here is the PR: https://github.com/GoogleCloudPlatform/cloud-run-button/pull/193