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(storage): Changed refFromUrl regex to exclude appspot.com #3775

Merged
merged 2 commits into from
Nov 10, 2020

Conversation

dackers86
Copy link
Member

Fix for issue #2753.

When extracting the url, regex currently excludes appspot.com. This has now been included so that the domain is included.

Format changed from {bucket} to {bucket}.appspot.com

phatmann
phatmann previously approved these changes Jun 11, 2020
@dackers86 dackers86 self-assigned this Jun 24, 2020
@dackers86 dackers86 requested review from Salakar, Ehesp, mikehardy and russellwheatley and removed request for mikehardy June 24, 2020 08:05
@dackers86 dackers86 changed the title Changed refFromUrl regex to exclude appspot.com fix(storage): Changed refFromUrl regex to exclude appspot.com Jul 31, 2020
@vercel
Copy link

vercel bot commented Sep 8, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/qkk91rk83
✅ Preview: https://react-native-firebase-git-dackers86-fix-ref-url-domain.invertase.vercel.app

@dackers86 dackers86 requested a review from mikehardy September 8, 2020 13:46
@mikehardy mikehardy added the Workflow: Needs Review Pending feedback or review from a maintainer. label Oct 27, 2020
dackers86 and others added 2 commits November 10, 2020 10:00
ios sends a 'storage/storage/file-not-found' while android sends 'storage/file-not-found'
pause/resume works locally but can fail in CI
canceling was timing out locally

Ideally there would be underlying fixes but the point of this PR is to re-enable the tests
in general, so having them running at all (in the majority) is a major win and worth getting in quickly
@mikehardy mikehardy force-pushed the @dackers86/fix-ref-url-domain branch from 1875a70 to 2a21f7a Compare November 10, 2020 16:00
@mikehardy mikehardy marked this pull request as ready for review November 10, 2020 16:00
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

@dackers86 finally unwinding some of the PR stack! This one looks fine, I de-merge-committed it, stripped it down to just the commits doing real changes and rebased it against master so it was clean+easy-to-review and it seems to pass everything locally

This is the one that re-enables the storage tests so I'm excited to get it in prior to the big breaking change about to land

Cheers

@codecov
Copy link

codecov bot commented Nov 10, 2020

Codecov Report

Merging #3775 (d624d26) into master (aca6992) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3775   +/-   ##
=======================================
  Coverage   41.92%   41.92%           
=======================================
  Files          35       35           
  Lines        1119     1119           
  Branches      278      278           
=======================================
  Hits          469      469           
  Misses        489      489           
  Partials      161      161           

@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar and removed Workflow: Needs Review Pending feedback or review from a maintainer. labels Nov 10, 2020
@mikehardy mikehardy merged commit c6f4699 into master Nov 10, 2020
@mikehardy mikehardy deleted the @dackers86/fix-ref-url-domain branch November 10, 2020 16:04
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Nov 10, 2020
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.

4 participants