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.

Sign up for GitHub

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

Jump to bottom

Allow deploying firestore without indexes #494

Merged
merged 2 commits into from
Oct 6, 2017
Merged

Allow deploying firestore without indexes #494

merged 2 commits into from
Oct 6, 2017

Conversation

samtstern
Copy link
Contributor

Fixes #493

@samtstern samtstern requested a review from mbleigh October 5, 2017 21:34
@samtstern samtstern assigned samtstern and mbleigh and unassigned samtstern Oct 5, 2017
@coveralls
Copy link

coveralls commented Oct 5, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 58.278% when pulling c2e33bb on ss-issue-493 into 945d6cb on master.

mbleigh
mbleigh approved these changes Oct 6, 2017
View reviewed changes
Copy link
Contributor

@mbleigh mbleigh left a comment

Choose a reason for hiding this comment

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

Approved, but please address my comment. 👍

lib/deploy/firestore/prepare.js Outdated
@@ -21,7 +21,11 @@ function _prepareRules(context, options) {
}

function _prepareIndexes(context, options) {
return context.firestoreIndexes ? firestoreIndexes.prepare(context, options) : RSVP.resolve();
if (!(context.firestoreIndexes && options.config.get('firestore.indexes'))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This negation is making my eyes cross, please rewrite as:

if (!context.firestoreIndexes || !options.config.get('firestore.indexes'))

@mbleigh mbleigh assigned samtstern and unassigned mbleigh Oct 6, 2017
@samtstern
Copy link
Contributor Author

@mbleigh comment addressed, please merge when ready (not sure about your merging policies on this repo)

@coveralls
Copy link

coveralls commented Oct 6, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 58.278% when pulling 8cb09de on ss-issue-493 into 945d6cb on master.

@samtstern samtstern removed their assignment Oct 6, 2017
@mbleigh mbleigh merged commit 3bbbe3e into master Oct 6, 2017
@mbleigh mbleigh deleted the ss-issue-493 branch October 6, 2017 17:11
joehan added a commit that referenced this pull request Nov 13, 2019
@joehan
Merge pull request #494 from FirebasePrivate/jh-local-info
7401383 
adds ext:info support for local extensions
yuchenshi pushed a commit that referenced this pull request Sep 18, 2024
@dpebot
Auto-update dependencies. (#494)
e74276e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbleigh mbleigh mbleigh approved these changes

Assignees

@mbleigh mbleigh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@samtstern @coveralls @mbleigh