Skip to content

Update changelog script #4719

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

aaruni96
Copy link
Member

@aaruni96 aaruni96 commented Mar 7, 2025

Updates the changelog workflow to allow for an optional basetag. Also pushes change log stuff to ignore list for CI

Resolves #4666
Resolves #4670
Resolves #4671

@aaruni96 aaruni96 added release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes backport 1.3.x backport for release branch 1.3 labels Mar 7, 2025
@aaruni96 aaruni96 requested a review from fingolfin March 7, 2025 20:36
@aaruni96
Copy link
Member Author

aaruni96 commented Mar 7, 2025

Result at #4720

@aaruni96 aaruni96 mentioned this pull request Mar 10, 2025
10 tasks
@aaruni96

This comment was marked as outdated.

@aaruni96 aaruni96 requested a review from fingolfin March 12, 2025 14:57
@thofma

This comment was marked as outdated.

@lgoettgens

This comment was marked as outdated.

fingolfin

This comment was marked as outdated.

@lgoettgens

This comment was marked as outdated.

@lgoettgens lgoettgens removed the backport 1.3.x backport for release branch 1.3 label Mar 14, 2025
@lgoettgens lgoettgens dismissed fingolfin’s stale review March 14, 2025 14:34

just talked and we don't merge this right now

@lgoettgens

This comment was marked as outdated.

Copy link

codecov bot commented May 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.89%. Comparing base (998fa2e) to head (a5b6a82).
Report is 27 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4719      +/-   ##
==========================================
+ Coverage   84.84%   84.89%   +0.05%     
==========================================
  Files         697      706       +9     
  Lines       94232    95341    +1109     
==========================================
+ Hits        79949    80940     +991     
- Misses      14283    14401     +118     

see 91 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

previous_minor = minor - 1
basetag = f"v{major}.{minor}dev"
basetag = f"{major}.{minor}dev"
Copy link
Member

Choose a reason for hiding this comment

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

Why drop the v?

Suggested change
basetag = f"{major}.{minor}dev"
basetag = f"v{major}.{minor}dev"

Copy link
Member Author

@aaruni96 aaruni96 Jul 3, 2025

Choose a reason for hiding this comment

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

This is due to an inconsistency (and I didn't look back far enough)

$ git tag --list | grep 'dev'
1.5dev
v1.4dev

The latest tag doesn't have the v in front of it. But 1.4 does (and I didn't look that far back, just tried making changelog for 1.5.0).
How do we handle this? Do we add back the v in code, and make a new tag identical to 1.5dev, but call it v1.5dev ?

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Looks good to me no, only don't understand why that one tag value was changed

@aaruni96
Copy link
Member Author

aaruni96 commented Jul 4, 2025

Summary after discussing privately with @fingolfin :

Thoughts @lgoettgens @benlorenz ?

@benlorenz
Copy link
Member

I think we should use the version with v and added the tag: https://github.com/oscar-system/Oscar.jl/releases/tag/v1.5dev

@lgoettgens
Copy link
Member

Seems like a human mistake on my side. I am happy to have either documentation or some automation regarding this to reduce the possibility of future similar errors

@aaruni96
Copy link
Member Author

I have restored the v, updated the documentation to reflect this, and update the github action.

@aaruni96
Copy link
Member Author

we should maybe adapt this script from GAP to make the release process more automated / less prone to human mistakes.

I propose we do that in a separate PR.

@aaruni96 aaruni96 requested a review from fingolfin July 10, 2025 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes
Projects
None yet
5 participants