GoogleCloudPlatform / apigee-samples

A repo for Apigee X/hybrid samples
Apache License 2.0
41 stars 44 forks source link

deploy script for basic-caching fails #154

Closed dfong closed 3 months ago

dfong commented 3 months ago

Describe the bug

there are two related aspects of this bug, both about the value of PROXY_URL.

  1. the README says to set PROXY_URL for the tests. however, since the deploy-basic-caching.sh script actually runs the test, the user must set it before calling that script as well. either the test commands should be removed from the deploy script, or the README should be changed to advise setting PROXY_URL before calling the deploy script. furthermore, since this value depends only on the incoming value of $APIGEE_HOST, why not have the scripts set it by default?

  2. the setup portion of the deploy script prints out:

All the Apigee artifacts are successfully deployed!

Your Proxy URL is: https://djfong-autopush-ngsaas.hybrid.e2e.apigeeks.net/v1/samples/basic-caching

which sounds like you could set the variable PROXY_URL to https://...

however, this value causes the tests to fail because they are not expecting the https:// prefix.

To Reproduce

Steps to reproduce the behavior:

run deploy-basic-caching.sh

Expected behavior

the script should run without errors, and exit with status 0.

Additional context

on a cloudtop VM. my org is in staging.

ssvaidyanathan commented 3 months ago

@dfong - I tried this on my Cloudtop instance and it worked as expected

The only issue I found is that the script at the end only prints part of the URL. It should have been

echo "To call the API, you can use the following command:"
echo " "
curl -S "https://$PROXY_URL?q=apigee&country=us"
echo " "
ssvaidyanathan commented 3 months ago

In the env.sh, for APIGEE_HOST, just put the domain and not include the protocol

For example

export APIGEE_HOST="example.com"
dfong commented 3 months ago

I tried this on my Cloudtop instance and it worked as expected

that's not what happens when i run the script. is it possible that it worked for you because your environment already had PROXY_URL set, prior running the script? could you please try doing "echo $PROXY_URL" before running it?

i note that the README doesn't say to set PROXY_URL. it says to set PROJECT, APIGEE_HOST, and APIGEE_ENV, (the standard env vars) which i did.

but later in the deploy script it calls the npm run test which apparently assumes that PROXY_URL has been set (which it hasn't).

i have seen this pattern in several of the samples so far: the deploy script runs the deployment, then prints to stdout that you are expected to set some variables before running tests. then it (the deploy script) runs test commands assuming that the variables have been set. but the problem is that the deploy script never gave the user the chance to set those variables.

i believe the basic-caching example has another problem (this is why i asked you to echo $PROXY_URL above). the PROXY_URL value printed by the deploy script begins with the https:// prefix, while the npm run test command actually assumes that it doesn't, leading to failure of the test.

the README agrees with the test assumption. but, i am trying to automate the running of all these scripts. so in my wrapper scripts, i want to dynamically extract the variable values from the deploy output. so the output has to be correct.

ssvaidyanathan commented 3 months ago

that's not what happens when i run the script. is it possible that it worked for you because your environment already had PROXY_URL set, prior running the script? could you please try doing "echo $PROXY_URL" before running it?

No - I setup my Cloudtop machine with the repo for the first time so that I can try and reproduce your issue

I have a feeling you had the protocol in the env.sh file for APIGEE_HOST and thats why you had this problem

so in my wrapper scripts, i want to dynamically extract the variable values from the deploy output. so the output has to be correct.

Not sure what script this is

ssvaidyanathan commented 3 months ago

Fixed