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.

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

Fix bug where release-please is looking at extra files from target branch instead of the one where changes are located #12

Conversation

dgellow
Copy link
Member

@dgellow dgellow commented Aug 29, 2023

Looking at the diff at https://github.com/stainless-sdks/sink-kotlin-public/pull/59/commits/a62d9a1df3030a35e27d6f3949afd951f2dd7010#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5 it seems that we aren't comparing against the correct branch when creating the changelog commit as part of a release PR. That means that any changes from extra-files (i.e a release-please config key specifying extra files to update) are reversed.

@dgellow dgellow marked this pull request as ready for review August 29, 2023 13:45
@@ -1280,7 +1226,7 @@ export class GitHub {
updates: Update[],
options?: CreatePullRequestOptions
): Promise<PullRequest> => {
const changes = await this.buildChangeSet(updates, baseBranch);
const changes = await this.buildChangeSet(updates, refBranch);
Copy link
Member Author

@dgellow dgellow Aug 29, 2023

Choose a reason for hiding this comment

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

@RobertCraigie I didn't test yet, but I believe this fixes the problem.

I will update the release branch and try again with the java sink repo, based on my changes.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome!

@dgellow dgellow changed the title Get rid of github wrappers assuming lookup from target branch Fix bug where release-please is looking at extra files from target branch instead of the one where changes are located Aug 29, 2023
this.addPath(yamlPath)
);
const contents: GitHubFileContents =
await this.github.getFileContentsOnBranch(
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think it would be cleaner to just use the changes branch as a default but I don't feel strongly.

Separately, does this PR mean that it's very difficult / impossible to do the wrong thing? i.e. you can't accidentally fetch from the wrong branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

I hope so, I checked for everything I could see that seems to be "defaultBranch", "targetBranch", "baseBranch" that I may have missed. But release-please has been built with the assumption the default branch is both the branch PRs should target against and the branch from which PRs, so it's still possible we may be missing a few other places.

@dgellow
Copy link
Member Author

dgellow commented Aug 29, 2023

@dgellow
Copy link
Member Author

dgellow commented Aug 29, 2023

And the release is correct too, just to be sure: https://github.com/stainless-sdks/sink-java-public/compare/v0.1.0...v0.1.1

I will merge this fix.

@dgellow dgellow merged commit 3c9d5d1 into main Aug 29, 2023
5 checks passed
@dgellow dgellow deleted the sam/sta-2088-release-please-changelog-compares-readme-against-main branch September 12, 2023 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants