Skip to content

Revert "Prevent pathological queries when doc is not specified"#797

Merged
cadamsdotcom merged 1 commit into
stagingfrom
cadams--align-staging-with-master
Nov 25, 2020
Merged

Revert "Prevent pathological queries when doc is not specified"#797
cadamsdotcom merged 1 commit into
stagingfrom
cadams--align-staging-with-master

Conversation

@cadamsdotcom

@cadamsdotcom cadamsdotcom commented Nov 25, 2020

Copy link
Copy Markdown
Contributor

This reverts commit 046fd47: "Prevent pathological queries when doc is not specified (#784)"

This is an emergency fix to restore admin functionality.

This reverts commit 046fd47.

This is an emergency fix to restore admin functionality.
@cadamsdotcom

Copy link
Copy Markdown
Contributor Author

@0xdevalias PTAL

@0xdevalias 0xdevalias left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved (retroactively).

Musing: I think for 'emergency fixes' we need a workflow where we can easily get them through to prod, even if there isn't necessarily someone around who can approve it.

My desire is that everything at least has a PR for visibility/documenting it/etc, but I don't necessarily think that needs to be reviewed/approved.

Have some thoughts i'll let brew on this and we can figure a good pattern/solution moving forward.

@0xdevalias 0xdevalias added the ❌ do-not-merge PR should not be merged. Ask the person who added it for more details if the reason isn't clear. label Nov 25, 2020
@0xdevalias

0xdevalias commented Nov 25, 2020

Copy link
Copy Markdown
Contributor

image

This highlights another issue with the way our current workflows are configured, at least for this use case. It's wanting us to make a squash merge for this.. but I think for a revert that may not be something we actually want to do.. not sure, what do you think? @thecadams

@cadamsdotcom cadamsdotcom merged commit a5e6923 into staging Nov 25, 2020
@cadamsdotcom cadamsdotcom deleted the cadams--align-staging-with-master branch November 25, 2020 04:46
@cadamsdotcom

Copy link
Copy Markdown
Contributor Author

Musing: I think for 'emergency fixes' we need a workflow where we can easily get them through to prod, even if there isn't necessarily someone around who can approve it.

My desire is that everything at least has a PR for visibility/documenting it/etc, but I don't necessarily think that needs to be reviewed/approved.

Yep we need ability to break glass. Asked here: https://stackoverflow.com/questions/64998944/ability-to-break-glass-and-bypass-github-branch-protection-for-emergency-fixes

@0xdevalias

0xdevalias commented Nov 25, 2020

Copy link
Copy Markdown
Contributor

That StackOverflow is great! Opened a draft PR at sparkletown/.github#1 where we can make this happen! 🎉

Edit: Also backlogged https://github.com/sparkletown/internal-sparkle-issues/issues/13 to implement some automated/bot-based stuff around our current manual break-glass methods.

@0xdevalias 0xdevalias changed the title Revert "Prevent pathological queries when doc is not specified (#784)" Revert "Prevent pathological queries when doc is not specified" Nov 25, 2020
0xdevalias added a commit that referenced this pull request Nov 25, 2020
@0xdevalias 0xdevalias removed the ❌ do-not-merge PR should not be merged. Ask the person who added it for more details if the reason isn't clear. label Nov 26, 2020
@0xdevalias 0xdevalias added the 🔨🍷 break-glass-approval-bypass Used to show that a 'break glass' approval bypass was used on this PR label Dec 5, 2020
mike-lvov added a commit that referenced this pull request Jan 14, 2021
…FirestoreConnect import (#801)

* Undo "Revert "Prevent pathological queries when doc is not specified (#784)" (#797)"

* Replace useParam usage with useVenueId across the codebase

* constrain useFirestoreConnect query types to enforce our firestore query patterns + update related usages (#1076)

Co-authored-by: Chris Adams <chris@cadams.com.au>
Co-authored-by: Mike Lvov <mike.lvov.soft@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨🍷 break-glass-approval-bypass Used to show that a 'break glass' approval bypass was used on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants