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

Influx: fix flux panic. cherry picks for #26329 #26331

Merged
merged 4 commits into from Jul 16, 2020
Merged

Influx: fix flux panic. cherry picks for #26329 #26331

merged 4 commits into from Jul 16, 2020

Conversation

ryantxu
Copy link
Member

@ryantxu ryantxu commented Jul 14, 2020

The go and test files in #26329 do not cleanly cherry pick to 7.1

This is a PR with explicit changes against 7.1. I'm not sure the best process here 🤷‍♂️

@ryantxu ryantxu requested a review from a team as a code owner July 14, 2020 18:21
@ryantxu ryantxu requested review from aknuds1 and jessabe and removed request for a team July 14, 2020 18:21
@ryantxu ryantxu added this to the 7.1 milestone Jul 14, 2020
@ryantxu ryantxu requested a review from dprokop July 14, 2020 18:22
@ryantxu
Copy link
Member Author

ryantxu commented Jul 14, 2020

@dprokop -- I added the cherry-pick label here since it needs to be added to 7.1, BUT note this pr is agains the 7.1.x branch since #26329 will not cherry pick cleanly.

What is the best practice here?

papagian
papagian reviewed Jul 15, 2020
View reviewed changes
Copy link
Contributor

@papagian papagian left a comment

Choose a reason for hiding this comment

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

LGTM

The CI fails to build enterprise because it targets the v7.1.x branch and tries to checkout an invalid OSS branch.
@dprokop We need to change it to target the cherry-picks PR once it will be created and merge it together with the others.

@dprokop
Copy link
Member

dprokop commented Jul 15, 2020

@papagian haven't we agreed to cherry pick #26329 instead?

@papagian
Copy link
Contributor

@papagian haven't we agreed to cherry pick #26329 instead?X

we cannot because the verifyGoldenResponse() that is called by the test does not exist in #26329 (it is introduced in another commit that it will not be included in 7.1)

@dprokop
Copy link
Member

dprokop commented Jul 15, 2020

OK, then i'll merge this one to the cherry picks branch 👍

aknuds1
aknuds1 approved these changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM, with a suggestion.

pkg/tsdb/influxdb/flux/executor_test.go Show resolved Hide resolved
@papagian papagian changed the base branch from v7.1.x to cp-7.1.0 July 16, 2020 09:54
@dprokop dprokop changed the base branch from cp-7.1.0 to master July 16, 2020 09:55
@dprokop dprokop requested a review from a team July 16, 2020 09:55
@dprokop dprokop requested a review from a team as a code owner July 16, 2020 09:55
@dprokop dprokop requested review from torkelo and besartberisha and removed request for a team July 16, 2020 09:55
@dprokop dprokop changed the base branch from master to cp-7.1.0 July 16, 2020 09:55
@papagian papagian mentioned this pull request Jul 16, 2020
Merged
@papagian papagian merged commit 286c62b into cp-7.1.0 Jul 16, 2020
@papagian papagian deleted the influx-fix-710 branch July 16, 2020 10:24
dprokop pushed a commit that referenced this pull request Jul 16, 2020
@ryantxu @dprokop
Influx: fix flux panic. cherry picks for #26329 (#26331)
8101355 
* influx fix for 7.1.0

* don't upgrade sdk

* revert go mod changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aknuds1 aknuds1 aknuds1 approved these changes

@papagian papagian papagian left review comments

@jessabe jessabe Awaiting requested review from jessabe

@dprokop dprokop Awaiting requested review from dprokop

@marcusolsson marcusolsson Awaiting requested review from marcusolsson

@oddlittlebird oddlittlebird Awaiting requested review from oddlittlebird

@torkelo torkelo Awaiting requested review from torkelo

@besartberisha besartberisha Awaiting requested review from besartberisha

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@ryantxu @dprokop @papagian @aknuds1