Fix upstream merge conflicts [DO NOT MERGE, just review] #62
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
|
as said in slack: The problem is this will either:
I think we need to merge in from HEAD unsquashed I think, and do the fixes in the single merge commit |
This comment has been minimized.
This comment has been minimized.
|
Maybe it helps to know how we keep https://github.com/grafana/mimir-prometheus/ up to date with github.com/prometheus/prometheus/ When we want to update mimir-prometheus with upstream changes, a separate PR is open, for example grafana/mimir-prometheus#287 This PR is always merged with a Merge commit not a squash. Then any changes performed on top of the mimir-prometheus fork are merged from separate prs and they can either be squashed commits or merged commits but usually they prefer squashed commits to keep 1 commit 1 pr parity. The benefits of this workflow is that you can always compare the fork to upstream and cherry pick any of the changes done in the fork in case you want to donate them back to upstream or just eventually sync up with upstream. |
|
@jesusvazquez are always all the merge conflicts fixes contained in the |
|
Ok this is what I'm thinking:
|
Any conflicts that may surface are addressed in the same PR as separate commits and then the entire PR is merged not squashed. |
ywwg
requested changes
Aug 10, 2022
| ret.Tags["name"] = metric | ||
| ret.PathExpression = ret.Name | ||
| ret.Values = a.Values | ||
| ======= | ||
| name := part[len(part)-1] |
There was a problem hiding this comment.
why are we not copying the tags or values?
There was a problem hiding this comment.
Those deleted lines were in our HEAD. Upstream deleted those and added the ret := a.CopyName(name) a few lines below: msaf1980@cf1f002
(this is the problem with this reviewing approach, the upstream changes are a little hidden. But I think overall it's better than a ~300 files PR)
| Want: []*types.MetricData{types.MakeMetricData("foo==.baz", | ||
| []float64{1, 2, 3, 4, 5}, 1, now32)}, | ||
| }, | ||
| // FIXME: I don't think `metric1.foo==.bar.baz` is a valid metric name, that is making this test fail |
| }, | ||
| }, | ||
| // FIXME: I don't think `..foo==.bar` is a valid metric name, that is making these testsapi/cmd/carbonapi/ht fail |
| r := a.CopyLink() | ||
| r.Name = tempName | ||
| r.Tags["name"] = r.Name | ||
| ======= | ||
| r := a.CopyName(tempName) |
There was a problem hiding this comment.
Does CopyName do the same changes that CopyLink did?
There was a problem hiding this comment.
| r := *a | ||
| r.Name = "nPercentile(" + a.Name + "," + percentStr + ")" | ||
| >>>>>>> upstream/main | ||
| r := a.CopyLink() |
There was a problem hiding this comment.
In some places, CopyName is used, and in some, CopyLink is used. What is the difference?
There was a problem hiding this comment.
CopyName is a new helper function the upstream introduced, check the behavior here: https://github.com/grafana/carbonapi/blob/npazosmendez/sync-upstream-fix-conflicts/expr/types/types.go#L452
I think it could be avoided. (there's lots of helpers that do almost the same thing (and sometimes the same thing): Copy, CopyLink, CopyName, CopyTag, CopyLinkTags, etc etc etc).
But I think changing that is out of the scope of this PR, wanted to leave the upstream changes as clean as possible.
There was a problem hiding this comment.
Looking forward to having CopyName be available! There's a lot of places where I've seen something like this happening:
md := origMd.CopyLink()
md.Name = "new name"
...
The problem with that is that the "name" tag then doesn't get updated correctly, but it looks like CopyName covers that.
| @@ -2,6 +2,7 @@ package removeBelowSeries | |||
|
|
|||
| import ( | |||
| "context" | |||
| "fmt" | |||
There was a problem hiding this comment.
is this import being used?
There was a problem hiding this comment.
Yes. The merge deleted it and I had to add it again to fix the build. The build would fail if it wasn't being used anyway.
| @@ -2,6 +2,7 @@ package smartSummarize | |||
|
|
|||
| import ( | |||
| "context" | |||
| "fmt" | |||
There was a problem hiding this comment.
is the import being used?
|
Go coverage report: Click to expand.
Go lint report: No issues found. 😎 |
npazosmendez
changed the title
npazosmendez
changed the title
ywwg
approved these changes
Aug 10, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Original PR: #60
This PR has the same changes but with a different workflow to make review easier. This is what I did:
npazosmendez/sync-upstream2and merged the upstream changes there. Commited everything blindly, with conflicts included.npazosmendez/sync-upstream2I branched tonpazosmendez/sync-upstream-fix-conflicts, where I compressed all the fixes to the conflicts in a single commit.npazosmendez/sync-upstream2<--npazosmendez/sync-upstream-fix-conflicts. This makes the diff of this PR just have the fixes to the conflicts and nothing elseOnce this PR is approved we can then merge
npazosmendez/sync-upstream2intomain.Note that I'm still missing some fixes. Those will come in separate commits, but the diff should still be clean.