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

Updates README for v0.2 and adds cloud run example instructions #21

Merged
merged 3 commits into from Jan 27, 2020

Conversation

bshaffer
Copy link
Collaborator

No description provided.

@bshaffer bshaffer requested review from grant and ace-n January 24, 2020 20:29
Copy link
Contributor

@ace-n ace-n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming everything is tested + works

README.md Outdated
-t gcr.io/$GCLOUD_PROJECT/my-cloud-function
```

> **NOTE**: Be sure to replace `$GCLOUD_PROJECT` with your Google Cloud project ID.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Often our instructions have them set that first, like:

export PROJECT_ID=$(gcloud config get-value core/project)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally have never seen this, can you point to an example?

Our docs usually have a <var> placeholder, and I've never done this for any PHP readmes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's more of the community tutorials that use the easy approach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it to the notes. I think it'd be better for them to explicitly set it, since it's possible they haven't called gcloud init yet, or they may be in a different project than the one they want to deploy.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines +130 to +131
docker build . \
-f vendor/google/cloud-functions-framework/examples/hello/Dockerfile \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be:

docker build examples/hello

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This path assumes the library is installed as a vendored dependency. Although even then, it would be cleaner as

docker build vendor/google/cloud-functions-framework/examples/hello \
    -t my-cloud-function

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I just cloned this repo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, also, if you do that it uses the index.php in the examples/hello repo, but if you follow the steps in the README, it expects you to create a custom index.php and use that.

The example can be used both ways. We could potentially outline BOTH in the readme (cloning the repo and build the example AND create your own and use composer). But it may be confusing to include both. WDYT?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Yeah, both approaches have value. I tend to create examples that are self-contained and runnable without modification.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cleaned up the steps so it's clearer that you are deploying one from an application directory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's the age-old tradeoff between ensuring the user does something and understands what they're doing, and getting something to work quickly. If they go in and run the example, they'll probably complete the steps faster but not have as strong of a grasp as to why. I think what we have here is a good compromise.

@bshaffer bshaffer merged commit 2c824dc into master Jan 27, 2020
@bshaffer bshaffer deleted the update-readme branch January 27, 2020 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants