consul
consul copied to clipboard
ci: Add PR number and title to oss-merge payload
Description
We want to use the titles from PRs in OSS when we create their accompanying Enterprise merges. With the title and the number in the trigger payload, we can consume it on the Enterprise side when we create the PR. This updates the trigger action to provide those values.
Testing & Reproduction steps
The biggest hurdle in testing here was dealing with the potential for there to be quotes or other special characters in the PR title that wouldn't play nicely when provided in a POST
request body to curl
. I ended up using the same method that we use for triggering a load test. This uses jq --arg
functionality to escape the values in the payload. One thing to note with this is that I had to remove hyphens from the arg names for it to work.
I'm not sure if this would work in the case where this triggers for PR OSS_0
, OSS_1
but on the enterprise side only OSS_1
gets merged with the changes of { OSS_0, OSS_1 }
.
One thing I found while trying solutions for myself was that the real commits do get merged over but are accompanied by 2 merge commits: 1 into the oss-merge branch and then another into main
when the PR is approved. So the screenshots of the 8 consecutive merge commits are actually empty merges (e.g. from someone merging OSS_0
when OSS_1
already had been merged)
I'm not sure if this would work in the case where this triggers for PR
OSS_0
,OSS_1
but on the enterprise side onlyOSS_1
gets merged with the changes of{ OSS_0, OSS_1 }
.
This is a good point. The way I have been treating the resulting PRs is that, since there should be a 1:1 now for each OSS PR that gets merged, each PR should get merged despite there being a backlog of other commits included in one when it gets created. Even if we unblock the head of the line and merge multiple changes in one, we can still have a reference for each OSS change (by virtue of its title and OSS SHA) even if it becomes a no-op.
Your point is valid though. Unless we have a way to apply the changes out-of-order these PR titles may be misleading when they contain more than one change.
I'm not sure if this would work in the case where this triggers for PR
OSS_0
,OSS_1
but on the enterprise side onlyOSS_1
gets merged with the changes of{ OSS_0, OSS_1 }
.
One more clarification about this... I think it should technically still "work" as the title is not pulled in on the Enterprise side from the git log
but rather it's provided by the OSS PR closed event. I think @kisunji points out that the titles will be misleading in the case he mentions because OSS_1
will be titled for a single PR, but contain the changes for { OSS_0, OSS_1 }
.
One way we could handle this wrinkle is by somehow indicating a PR is a "rollup" of many OSS PRs, but that will require some much more work on the Enterprise side to handle and I don't think it would be pretty.
Another we we could handle this is to ensure that each Enterprise PR only contains a single OSS PR either by "social" contract (aka manually merging them in order) or by complex software we write on the Enterprise side (arm wavy something something maybe it's possible?)
This is a good point. The way I have been treating the resulting PRs is that, since there should be a 1:1 now for each OSS PR that gets merged, each PR should get merged despite there being a backlog of other commits included in one when it gets created. Even if we unblock the head of the line and merge multiple changes in one, we can still have a reference for each OSS change (by virtue of its title and OSS SHA) even if it becomes a no-op.
Not sure if I agree here or if I'm misunderstanding;
We need 1:1 commits to exist, but if one mega-backlogged PR gets merged containing 100 commits from OSS, technically all the other previous PRs don't need to be merged (and will be empty merge commits which I'd want to avoid)
Commits between OSS and ENT need to be 1:1 but PR don't need to as we can have only 1 PR that merge multiple changes from OSS to ENT.
Currently we create a merge for each Push to OSS but we can decide to make it happen every night for example (not saying we should though 😅).
I think passing those information to enterprise is a good thing but we need to add them as text in the PR body on the enterprise side to make sure it's not misleading. Something like:
latest merged OSS PR ported to enterprise:
ci: Add PR number and title to oss-merge payload #14283
This pull request has been automatically flagged for inactivity because it has not been acted upon in the last 60 days. It will be closed if no new activity occurs in the next 30 days. Please feel free to re-open to resurrect the change if you feel this has happened by mistake. Thank you for your contributions.
Closing due to inactivity. If you feel this was a mistake or you wish to re-open at any time in the future, please leave a comment and it will be re-surfaced for the maintainers to review.