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 changelog requirement action for major/minor bumps #174

Merged
merged 19 commits into from
Jun 10, 2024

Conversation

emilypgoogle
Copy link
Collaborator

@emilypgoogle emilypgoogle commented Jun 6, 2024

"Warn Version Bump" will fail if a PR contains a major/minor bump without a changelog. There most recent test commits show what this failure looks like.

@emilypgoogle emilypgoogle requested review from rlazo and daymxn June 6, 2024 20:11
@emilypgoogle emilypgoogle marked this pull request as ready for review June 6, 2024 20:11
@rlazo
Copy link
Collaborator

rlazo commented Jun 6, 2024

When running the task locally from within this branch I get

* What went wrong:
Execution failed for task ':common:warnVersionBump'.
> Changes are MAJOR, higher than PATCH. If this is intended, add a changelog entry. Otherwise, revert the changes.

Is this expected?

@emilypgoogle
Copy link
Collaborator Author

Like the check for API changes task, there needs to be context for what API state to compare against. The action sets up the previous state to be the target branch, the local state may vary.

@rlazo
Copy link
Collaborator

rlazo commented Jun 6, 2024

Like the check for API changes task, there needs to be context for what API state to compare against. The action sets up the previous state to be the target branch, the local state may vary.

What I'm trying to verify is if the task :

a) correctly identifies the expected version bump
b) needs any additional context to work

My local state is: it's a new repo clone with no local changes. Under these conditions, is the major version warning message expected?

@emilypgoogle
Copy link
Collaborator Author

Yes, I believe it's diffing an empty API with the modern API, which will always be major. For local testing, you'd need to run ./gradlew exportApi for whatever base state you want and then this task would compare against that.

@rlazo
Copy link
Collaborator

rlazo commented Jun 6, 2024

Yes, I believe it's diffing an empty API with the modern API, which will always be major. For local testing, you'd need to run ./gradlew exportApi for whatever base state you want and then this task would compare against that.

You're right. Running ./gradlew exportApi and then ./gradlew warnVersionBump shows no warning, as expected. Could you make this behaviour more explicit? For example, by printing what it's comparing, or by failing the task if there's no file to compare with.

@emilypgoogle emilypgoogle enabled auto-merge (squash) June 10, 2024 18:41
rlazo
rlazo previously approved these changes Jun 10, 2024
.github/workflows/warn-version-bump.yml Outdated Show resolved Hide resolved
.github/workflows/warn-version-bump.yml Outdated Show resolved Hide resolved
@emilypgoogle emilypgoogle merged commit 6b406cf into main Jun 10, 2024
6 checks passed
@emilypgoogle emilypgoogle deleted the ep/check-api-changes branch June 10, 2024 23:03
PatilShreyas pushed a commit to PatilShreyas/generative-ai-kmp that referenced this pull request Sep 21, 2024
…#174)

"Warn Version Bump" will fail if a PR contains a major/minor bump
without a changelog. There most recent test commits show what this
failure looks like.
PatilShreyas pushed a commit to PatilShreyas/generative-ai-kmp that referenced this pull request Sep 21, 2024
…#174)

"Warn Version Bump" will fail if a PR contains a major/minor bump
without a changelog. There most recent test commits show what this
failure looks like.
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