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

Fix Hosting deploys #5077

Merged
merged 3 commits into from Oct 6, 2022
Merged

Fix Hosting deploys #5077

merged 3 commits into from Oct 6, 2022

Conversation

bkendall
Copy link
Contributor

@bkendall bkendall commented Oct 6, 2022

Description

  1. TypeError: Cannot read properties of undefined (reading 'join')

An import that was import path from "path" wasn't working correctly.

  1. Error: HTTP Error: 403, Permission 'cloudfunctions.functions.list' denied deploying Hosting

Logic was added to check for backends but no assertion was made that the callee would have permissions. Added a check to make sure that (1) we had rewrites we should try to resolve and (b) catch permissions issues and silence them.

Fixes #5071

Copy link
Member

@inlined inlined left a comment

Choose a reason for hiding this comment

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

Curious regression. I'm wondering how this was avoided in the original code (maybe it already did the respective check)

@TheIronDev TheIronDev self-requested a review October 6, 2022 18:01
Copy link
Contributor

@TheIronDev TheIronDev left a comment

Choose a reason for hiding this comment

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

LGTM!

@bkendall bkendall marked this pull request as ready for review October 6, 2022 18:02
@bkendall bkendall enabled auto-merge (squash) October 6, 2022 18:02
@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2022

Codecov Report

Base: 55.75% // Head: 55.75% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (6bbb3ce) compared to base (97e0d04).
Patch coverage: 63.63% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5077      +/-   ##
==========================================
- Coverage   55.75%   55.75%   -0.01%     
==========================================
  Files         305      305              
  Lines       20513    20522       +9     
  Branches     4139     4142       +3     
==========================================
+ Hits        11437    11442       +5     
- Misses       8093     8097       +4     
  Partials      983      983              
Impacted Files Coverage Δ
src/deploy/hosting/convertConfig.ts 68.62% <60.00%> (-1.27%) ⬇️
src/hosting/config.ts 66.14% <100.00%> (ø)
src/emulator/auth/state.ts 84.87% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines +85 to +87
* TODO: this currently lists remote backends (functions) and attemtps to validate them.
* We currently catch 403 issues and handle them, but it's probably not the best solution
* to have a required permission in functions when a deploy may "only" be to Hosting.
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: convert this to an issue

Copy link
Contributor

@steveoh steveoh left a comment

Choose a reason for hiding this comment

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

Thanks!

@bkendall bkendall merged commit 1cc23a3 into master Oct 6, 2022
christhompsongoogle pushed a commit that referenced this pull request Oct 6, 2022
* fix path import

* adds check for permission issues when resolving Hosting configs
@drummerjolev
Copy link

Thanks @bkendall! Any ETA on the version release? I'm using the Firebase GH action, which has been broken since this morning.

Though FirebaseExtended/action-hosting-deploy#242 could also be a solution.

@bkendall bkendall deleted the bk-fixes2 branch October 6, 2022 20:21
tamalsaha added a commit to appscode/blog that referenced this pull request Oct 6, 2022
xref: firebase/firebase-tools#5077

Signed-off-by: Tamal Saha <tamal@appscode.com>
tamalsaha added a commit to appscode/blog that referenced this pull request May 17, 2024
xref: firebase/firebase-tools#5077

Signed-off-by: Tamal Saha <tamal@appscode.com>
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.

Failed to deploy to hosting with 'Failed to list functions for project' error
6 participants