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

functions/composer-storage-trigger/index.js sends nonexistent argument "iap.jwt" to "makeIapPostRequest" #1602

Closed
starmandeluxe opened this issue Feb 9, 2020 · 4 comments
Assignees
Labels
api: composer Issues related to the Cloud Composer API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@starmandeluxe
Copy link

starmandeluxe commented Feb 9, 2020

It looks like the final argument in makeIapPostRequest ("jwt") was removed at some point (43cb78c#diff-03c121b657e9255e31ace2345598d678R152), but for some reason the code sample still provides it in the call to this function:

Why?

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Feb 10, 2020
@fhinkel fhinkel added api: serverless priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed triage me I really want to be triaged. labels Feb 11, 2020
@fhinkel
Copy link
Contributor

fhinkel commented Feb 11, 2020

@ace-n , could you have a look at this functions sample? Thanks

@ace-n
Copy link
Contributor

ace-n commented Feb 12, 2020

@leahecole owns this sample (per CODEOWNERS) - over to you 🙂

@fhinkel fhinkel added api: storage Issues related to the Cloud Storage API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. and removed api: serverless priority: p2 Moderately-important priority. Fix may not be included in next release. labels Mar 24, 2020
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Mar 30, 2020
@meredithslota meredithslota added api: composer Issues related to the Cloud Composer API. priority: p2 Moderately-important priority. Fix may not be included in next release. and removed 🚨 This issue needs some love. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. labels Apr 22, 2020
@meredithslota
Copy link
Contributor

I'm not sure if this makes sense but I'm adding the "composer" label so this is surfaced to our team, and dropping priority to P2.

@meredithslota meredithslota removed the api: storage Issues related to the Cloud Storage API. label Apr 22, 2020
@leahecole
Copy link
Contributor

I'm investigating this now. I tried running the code in the relevant tutorial as is, and it seems to work - the DAG triggers and prints the appropriate info.

I retried adding in the jwt parameter like it was before, and the DAG triggered then too, printing the appropriate info. This occurred because the makeIapPostRequest function never reads this value. The fact that the function never reads the value is likely why it was removed in this PR that was running eslint - it removed unused parameters. The line that you highlighted managed to get leftover somehow, but has since been updated. There are now no longer any references to this unused jwt - the ones that remain are used in the service account signature request.

Thank you for opening this - I'm going to close it because as far as I can tell, the code doesn't need any updates at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: composer Issues related to the Cloud Composer API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

6 participants