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

feat: add TypeScript samples support #3029

Merged
merged 77 commits into from
Feb 23, 2023
Merged

feat: add TypeScript samples support #3029

merged 77 commits into from
Feb 23, 2023

Conversation

kweinmeister
Copy link
Collaborator

@kweinmeister kweinmeister commented Feb 10, 2023

This PR enables adding TypeScript samples without managing duplicate JS and TS versions.

  • It uses the typeless-sample-bot to parse the .ts samples into .js in post-processing.
  • OwlBot is needed to trigger the bot. owlbot.py runs the bot and updates the formatting. It bypasses the owlbot_main() flow that performs client library tasks.
  • The migration of a small sample scheduler was included to test the process. Also, the existing appengine/typescript sample needed to be updated to reconcile with the root level .eslintrc.json.
  • CONTRIBUTING.md was updated with TypeScript instructions.

@kweinmeister kweinmeister requested a review from a team as a code owner February 10, 2023 20:23
@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Feb 10, 2023
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

I think that owlbot-nodejs-mono-repo will have some unintended consequences, because it does things like updating the README.md and copying over templates.

What we can do is create a trimmed down version here that strips out the logic for templates, etc., and keeps just the typeless-sample bits.

@kweinmeister kweinmeister marked this pull request as draft February 10, 2023 20:56
@kweinmeister kweinmeister added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 11, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 11, 2023
@kweinmeister kweinmeister marked this pull request as ready for review February 13, 2023 14:54
@kweinmeister kweinmeister requested a review from a team as a code owner February 15, 2023 18:27
@snippet-bot
Copy link

snippet-bot bot commented Feb 15, 2023

Here is the summary of changes.

You are about to add 2 region tags.
You are about to delete 1 region tag.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@kweinmeister kweinmeister marked this pull request as draft February 15, 2023 18:31
@kweinmeister kweinmeister added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 15, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 15, 2023
@kweinmeister kweinmeister added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2023
@kweinmeister kweinmeister added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2023
@kweinmeister kweinmeister added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2023
@kweinmeister kweinmeister marked this pull request as ready for review February 17, 2023 16:28
@kweinmeister kweinmeister merged commit 8fc8d3a into main Feb 23, 2023
@kweinmeister kweinmeister deleted the owlbot branch February 23, 2023 17:15
"start": "node ./index.js",
"gcp-build": "tsc -p .",

Choose a reason for hiding this comment

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

Was this removed intentionally, the documentation still refers to this as an option for running an custom build script: https://cloud.google.com/appengine/docs/standard/nodejs/running-custom-build-step#example

Copy link
Member

Choose a reason for hiding this comment

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

I am pretty sure this was a mistake. The docs are correct.

Copy link
Member

@matthewrobertson matthewrobertson Mar 2, 2023

Choose a reason for hiding this comment

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

This change actually broke the ability to deploy to GAE. It now fails in the build phase with an error like:

ERROR: (gcloud.app.deploy) Error Response: [9] Cloud build cdec98ca-e4aa-4a97-bbfc-d5843c7ff8a3 status: FAILURE
> appengine-typescript@0.0.1 prepare
> npm run compile


> appengine-typescript@0.0.1 compile
> tsc -p .

sh: 1: tsc: not found
npm ERR! code 127
npm ERR! path /workspace
npm ERR! command failed
npm ERR! command sh -c -- npm run compile

npm ERR! A complete log of this run can be found in:

This is because the devDependencies are not installed when the prepare script is run. The gcp-build script fixes this.

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 change has been reverted, and a test with npm run deploy was successful.

Copy link
Member

Choose a reason for hiding this comment

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

FYI, we are currently working on a change to run npm run build by default when deploying to AppEngine, because this gcp-buildthing is not very intuitive.

cc @anniefu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants