Composer: Move 'triggering DAGs' sample to GitHub. #716
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note: as this is moving code as-is from cloud.google.com, I don't add tests in this PR. The only changes I made were to fix the linter errors. I have an open bug 111401746 to fix the error handling code and plan to add unit tests at that time. |
Codecov Report
@@ Coverage Diff @@
## master #716 +/- ##
=======================================
Coverage 55.17% 55.17%
=======================================
Files 1 1
Lines 58 58
=======================================
Hits 32 32
Misses 26 26Continue to review full report at Codecov.
|
ace-n
suggested changes
Aug 23, 2018
|
|
||
| 'use strict'; | ||
|
|
||
| // [START composer_trigger] |
There was a problem hiding this comment.
Nit: functions region tags are (usually) prefixed with functions_.
There was a problem hiding this comment.
I'd be happy to, but are you a samples lead in go/samples-tracker? I don't have permissions to make functions samples there.
| exports.triggerDag = function triggerDag (event, callback) { | ||
| // Fill in your Composer environment information here. | ||
|
|
||
| // The project that holds your function |
There was a problem hiding this comment.
Nit - typically:
- we only show the contents of a function within a sample (i.e. we omit the function signature)
- we pass in any required values as parameters
- we include definitions of those values in the body of the function, but they are commented out
Apply this comment throughout your PR (if you choose to).
There was a problem hiding this comment.
Even for a cloud functions sample? This sample is intended to be copy-pasted into the cloud functions web UI, so I believe the function signature is necessary.
| const DAG_NAME = 'composer_sample_trigger_response_dag'; | ||
|
|
||
| // Other constants | ||
| const WEBSERVER_URL = 'https://' + WEBSERVER_ID + |
There was a problem hiding this comment.
Nit: use template literals to construct strings?
| // Make the request | ||
| authorizeIap( | ||
| CLIENT_ID, PROJECT_ID, USER_AGENT, | ||
| function iapAuthorizationCallback (err, jwt, idToken) { |
There was a problem hiding this comment.
Can we use promises instead of callbacks here? (Both versions of Node on GCF support promises, though only Node 8 supports async/await.)
There was a problem hiding this comment.
Done. I switched to the node-fetch library which does support promises.
|
|
||
| const SERVICE_ACCOUNT = [projectId, '@appspot.gserviceaccount.com'].join(''); | ||
|
|
||
| var options = { |
There was a problem hiding this comment.
Can we use const or let instead of var?
There was a problem hiding this comment.
Fixed in new node-fetch logic.
Sample was originally hard-coded in HTML at https://cloud.google.com/composer/docs/how-to/using/triggering-with-gcf
322ee96 to
677cf79
Compare
TrevorEdwards
approved these changes
Aug 29, 2018
ace-n
pushed a commit
that referenced
this pull request
Aug 30, 2018
* Composer: Move 'triggering DAGs' sample to GitHub. Sample was originally hard-coded in HTML at https://cloud.google.com/composer/docs/how-to/using/triggering-with-gcf * Use node-fetch for promises in HTTP requests.
ace-n
pushed a commit
that referenced
this pull request
Nov 16, 2022
🤖 I have created a release *beep* *boop* --- ## [4.0.0](googleapis/nodejs-video-intelligence@v3.4.1...v4.0.0) (2022-06-29) ### ⚠ BREAKING CHANGES * update library to use Node 12 (#714) * field ObjectTrackingAnnotation.segment moved into oneof, added track_id (#704) ### Features * field ObjectTrackingAnnotation.segment moved into oneof, added track_id ([#704](googleapis/nodejs-video-intelligence#704)) ([b55757b](googleapis/nodejs-video-intelligence@b55757b)) * support regapic LRO ([#720](googleapis/nodejs-video-intelligence#720)) ([903e5ca](googleapis/nodejs-video-intelligence@903e5ca)) ### Bug Fixes * fixes for dynamic routing and streaming descriptors ([#716](googleapis/nodejs-video-intelligence#716)) ([853d91e](googleapis/nodejs-video-intelligence@853d91e)) ### Build System * update library to use Node 12 ([#714](googleapis/nodejs-video-intelligence#714)) ([a9fbbcd](googleapis/nodejs-video-intelligence@a9fbbcd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
ace-n
pushed a commit
that referenced
this pull request
Nov 17, 2022
🤖 I have created a release *beep* *boop* --- ## [4.0.0](googleapis/nodejs-video-intelligence@v3.4.1...v4.0.0) (2022-06-29) ### ⚠ BREAKING CHANGES * update library to use Node 12 (#714) * field ObjectTrackingAnnotation.segment moved into oneof, added track_id (#704) ### Features * field ObjectTrackingAnnotation.segment moved into oneof, added track_id ([#704](googleapis/nodejs-video-intelligence#704)) ([b55757b](googleapis/nodejs-video-intelligence@b55757b)) * support regapic LRO ([#720](googleapis/nodejs-video-intelligence#720)) ([903e5ca](googleapis/nodejs-video-intelligence@903e5ca)) ### Bug Fixes * fixes for dynamic routing and streaming descriptors ([#716](googleapis/nodejs-video-intelligence#716)) ([853d91e](googleapis/nodejs-video-intelligence@853d91e)) ### Build System * update library to use Node 12 ([#714](googleapis/nodejs-video-intelligence#714)) ([a9fbbcd](googleapis/nodejs-video-intelligence@a9fbbcd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
ahrarmonsur
pushed a commit
that referenced
this pull request
Nov 17, 2022
🤖 I have created a release *beep* *boop* --- ## [4.0.0](googleapis/nodejs-video-intelligence@v3.4.1...v4.0.0) (2022-06-29) ### ⚠ BREAKING CHANGES * update library to use Node 12 (#714) * field ObjectTrackingAnnotation.segment moved into oneof, added track_id (#704) ### Features * field ObjectTrackingAnnotation.segment moved into oneof, added track_id ([#704](googleapis/nodejs-video-intelligence#704)) ([b55757b](googleapis/nodejs-video-intelligence@b55757b)) * support regapic LRO ([#720](googleapis/nodejs-video-intelligence#720)) ([903e5ca](googleapis/nodejs-video-intelligence@903e5ca)) ### Bug Fixes * fixes for dynamic routing and streaming descriptors ([#716](googleapis/nodejs-video-intelligence#716)) ([853d91e](googleapis/nodejs-video-intelligence@853d91e)) ### Build System * update library to use Node 12 ([#714](googleapis/nodejs-video-intelligence#714)) ([a9fbbcd](googleapis/nodejs-video-intelligence@a9fbbcd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sample was originally hard-coded in HTML at
https://cloud.google.com/composer/docs/how-to/using/triggering-with-gcf
Python code for target DAG is also being moved to GitHub in GoogleCloudPlatform/python-docs-samples#1645
@TrevorEdwards Please review.