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

chore: remove queryBuilderGrouping flag #16711

Merged
merged 1 commit into from
Feb 4, 2020
Merged

chore: remove queryBuilderGrouping flag #16711

merged 1 commit into from
Feb 4, 2020

Conversation

hoorayimhelping
Copy link
Contributor

@hoorayimhelping hoorayimhelping commented Feb 3, 2020 •
edited
Loading

Removes the queryBuilderGrouping flag, effectively turning the feature on.

hoorayimhelping
hoorayimhelping commented Feb 3, 2020
View reviewed changes
ui/src/timeMachine/utils/queryBuilder.ts
return query
}

function buildQueryHelperButWithGrouping(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this diff is wonky. i renamed buildQueryHelperButWithGrouping -> buildQueryFromConfig and removed buildQueryHelper which I thought could have been named a little clearly (hence buildQueryFromConfig)

@hoorayimhelping hoorayimhelping requested review from ebb-tide and a team February 3, 2020 20:18
@ghost ghost removed their request for review February 3, 2020 20:18
@hoorayimhelping hoorayimhelping requested a review from a team February 3, 2020 20:19
@ghost ghost removed their request for review February 3, 2020 20:19
drdelambre
drdelambre approved these changes Feb 3, 2020
View reviewed changes
Copy link
Contributor

@drdelambre drdelambre left a comment

Choose a reason for hiding this comment

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

Code looks great.
whole thing makes me wonder if there should be a two stage approach (remove the flag, then revert the code at a later time) to allow for quicker rollbacks, but this might just be an artifact of not having a feature flagging service, so i'm not gonna think about it any more.

@hoorayimhelping hoorayimhelping force-pushed the bs_turn_on_grouping branch 2 times, most recently from e4b38cf to eb643fe Compare February 3, 2020 21:54
@hoorayimhelping hoorayimhelping changed the title chore: turn on queryBuilderGrouping flag chore: remove queryBuilderGrouping flag Feb 3, 2020
@hoorayimhelping
Copy link
Contributor Author

hoorayimhelping commented Feb 3, 2020 •
edited
Loading

@drdelambre I think you're right, the point of a flagging system is to set it to true, and if it doesn't work, set it to back to false, and I think this might be a little bit much.

So I started a new branch that just flips the flag the flag to true, and all these same tests fail. It seems like changing the flag from false to true, is functionally the same as removing the flag completely, in this instance.

@hoorayimhelping hoorayimhelping merged commit dd0ab8f into master Feb 4, 2020
@hoorayimhelping hoorayimhelping deleted the bs_turn_on_grouping branch February 4, 2020 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drdelambre drdelambre drdelambre approved these changes

@ebb-tide ebb-tide Awaiting requested review from ebb-tide

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hoorayimhelping @drdelambre