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

Cloud Composer DAG trigger sample uses to-be-deprecated JS #1260

Closed
starmandeluxe opened this issue May 3, 2019 · 13 comments
Closed

Cloud Composer DAG trigger sample uses to-be-deprecated JS #1260

starmandeluxe opened this issue May 3, 2019 · 13 comments
Assignees

Comments

@starmandeluxe
Copy link

The code sample for triggering a Cloud Composer DAG using a Cloud Function should be refactored to support nodejs8 since it is using nodejs6 which will be deprecated on April 22, 2020.

@fhinkel
Copy link
Contributor

fhinkel commented May 8, 2019

cc @ace-n

@ace-n
Copy link
Contributor

ace-n commented May 17, 2019

cc @stew-r

@stew-r
Copy link
Contributor

stew-r commented May 21, 2019

Is this sample actually used anywhere on the Cloud Functions docs?

Google Search doesn't show me any instances where it is being used. If we're not showing this sample in our docs, we can kill it.

@ace-n
Copy link
Contributor

ace-n commented May 21, 2019

It's used on this page.

@tswast is this something we can upgrade to Node 8?

@tswast
Copy link
Contributor

tswast commented May 21, 2019

Yes, please do upgrade the Composer DAG trigger sample.

CC @leahecole

@stew-r
Copy link
Contributor

stew-r commented May 21, 2019 via email

@leahecole
Copy link
Contributor

Ack. Definitely can do that upgrade v. soon - it's related to some work I'm doing with that sample.

@leahecole leahecole self-assigned this May 21, 2019
@ace-n
Copy link
Contributor

ace-n commented May 21, 2019

Updated linting rules to check for Node 8 here. Will work with @fhinkel to iterate on these - hopefully that'll wrap up by EOD today.

@leahecole
Copy link
Contributor

Great - I'll hold off until that's merged. Thanks so much for this!

@leahecole
Copy link
Contributor

leahecole commented May 23, 2019

Any updates on when this will be merged @ace-n ? Trying to decide if I should wait for the linter or if I should just go for it and do the changes without the linter.

@ace-n
Copy link
Contributor

ace-n commented May 23, 2019

I'd go for it now. The linter changes should be merged + added to CI anyway by the time you're done, so if you merge master into your branch + check CI they'll show up there (likely as WARNINGs - so CI may still pass). 🙂

@leahecole
Copy link
Contributor

Just as an FYI I submitted a CL to update the screenshot in the tutorial to also suggest using Node 8 - working with @ace-n to convert code appropriately

@ace-n
Copy link
Contributor

ace-n commented Jun 5, 2019

This was updated in #1320 and #1321 (thanks @leahecole!) - marking as fixed.

@ace-n ace-n closed this as completed Jun 5, 2019
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

No branches or pull requests

6 participants