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

Add global-revisions flag #158

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

felixjung
Copy link

I'm trying to include openapi-changes in our GitHub Actions CI to detect breaking changes in pull requests. My approach to doing this was to take the number of commits made on a branch n (given by github.event.pull_request.commits) and passing n+1 to openapi-changes via the --limit flag.

Problem
What I found was that the tool looks at the full local git history of the file, and uses the --limit flag to cut off after n commits. This leads to problems when a PR has 10 commits, but none or only some changed the OpenAPI spec being checked.

Approach
As a quick fix, this PR adds a global-revisions boolean flag, which is used to instruct git to list the history between specific revisions (i.e., HEAD~(n+1)..HEAD).

Alternative Approaches
I've seen #85 and I think the approach of passing git revisions directly makes a log of sense. However, I think there's some ambiguity around how this should be implemented.

  • One could pass two revisions and do a left-right comparison.
  • One could pass two revisions and do a report for every commit between them, as is the current behavior.
  • One could take a full revisions argument string that matches the supported inputs of git log itself.

All these are worth exploring, but I needed a quick win.

@felixjung felixjung marked this pull request as ready for review August 31, 2024 10:37
@felixjung felixjung changed the title Add repo-revisions flag Add global-revisions flag Sep 11, 2024
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.

1 participant