-
-
Notifications
You must be signed in to change notification settings - Fork 175
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
Support configuration of credentials with a config array #202
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking the time to open this PR, this will be a welcome improvement and much appreciated! 🙏🏻
Most of my change request address introduced whitespaces, blank lines and other formatting.
The relevant changes are
- I would prefer the introduction of
Arr
in a separate PR so that this PR is only about the added functionality. - I would move the checks into the
resolveCredentials()
method.
Out of curiosity: do you see a way to let users of the package now that using file
should be considered deprecated other than adding a comment? Either way, I'm going to just™ remove it in the next major version.
Thanks again!
Hi @jeromegamez, I think it's ready for a second look.
I renamed this into
Either log something with ✌️ |
Thanks for bearing with me (honestly), even though some requests were inconsistent 😅 There was a problem with the codecov action (codecov/codecov-action#1089) which I fixed in the main branch, could you rebase the PR so that the workflows can run again? |
Co-authored-by: Jérôme Gamez <github@jerome.gamez.name>
Co-authored-by: Jérôme Gamez <github@jerome.gamez.name>
Co-authored-by: Jérôme Gamez <github@jerome.gamez.name>
Co-authored-by: Jérôme Gamez <github@jerome.gamez.name>
Co-authored-by: Jérôme Gamez <github@jerome.gamez.name>
Co-authored-by: Jérôme Gamez <github@jerome.gamez.name>
Co-authored-by: Jérôme Gamez <github@jerome.gamez.name>
Uh, nice, I just discovered the Rebase button in the web view of a PR! If this works, I'll merge! |
Codecov Report
@@ Coverage Diff @@
## main #202 +/- ##
=========================================
Coverage 98.41% 98.41%
- Complexity 47 48 +1
=========================================
Files 3 3
Lines 126 126
=========================================
Hits 124 124
Misses 2 2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Thanks again! 🙏🏻 |
Nice one thanks. Would you be able to publish a new release please? |
Done ✅ |
Description
On our applications, in order to have more granularity in the configuration of the credentials, we had to extend the package to be able to optionally receive the configuration of the credentials via an array instead of a JSON file.
This makes it easier to integrate with our CI/CD pipelines and also gives us the ability to cache the config via Laravel.
This is a change we think could be valuable to others too, especially considering that
firebase-php
already supports arrays,Note: we slightly tweaked the expected configuration array for
credentials
but don't worry this is a non-breaking change.Checklist: