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.

Sign up for GitHub

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

Jump to bottom

Include commits since last release in RPM build number #7896

Merged
merged 5 commits into from
Jan 11, 2024
Merged

Include commits since last release in RPM build number #7896

merged 5 commits into from
Jan 11, 2024

Conversation

zrhoffman
Copy link
Member

This PR changes the build number to include the number of commits since the last release, rather than the total number of commits ever. Previously, the total commit count was used to ensure that a build from a later child commit will always be considered a higher version number.

But because the version number increases each time the release version increases, counting only the commits since the last release tag is sufficient.

Which Traffic Control components are affected by this PR?

  • Documentation
  • CDN in a Box
  • Traffic Portal v2

What is the best way to verify this PR?

  • Test /about TO API endpoint
  • Make sure the CDN-in-a-Box CI GHA workflow picks up the already-built RPMs instead of rebuilding them all again.

PR submission checklist

@zrhoffman zrhoffman added documentation related to documentation cdn-in-a-box related to the Docker-based CDN-in-a-Box system build related to the build process Traffic Portal v2 Related to the experimental Traffic Portal version 2 labels Jan 9, 2024
@codecov Codecov
Copy link

codecov bot commented Jan 9, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a28af5a) 31.80% compared to head (14122bb) 65.65%.
Report is 3 commits behind head on master.

Additional details and impacted files 
@@              Coverage Diff              @@
##             master    #7896       +/-   ##
=============================================
+ Coverage     31.80%   65.65%   +33.84%     
  Complexity       98       98               
=============================================
  Files           719      323      -396     
  Lines         82819    12836    -69983     
  Branches        970      970               
=============================================
- Hits          26340     8427    -17913     
+ Misses        54320     4050    -50270     
+ Partials       2159      359     -1800
Flag Coverage Δ
golib_unit ?
grove_unit ?
t3c_unit ?
traffic_monitor_unit ?
traffic_ops_unit ?
traffic_portal_v2 74.37% <ø> (+0.01%) ⬆️
traffic_stats_unit ?
unit_tests 74.37% <ø> (+45.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zrhoffman zrhoffman force-pushed the relative-build-number branch 2 times, most recently from 8e0fc78 to c9a6e4f Compare January 10, 2024 15:48
@zrhoffman
Use number of commits since last release in build number
650fd46 
Previously, the number of commits ever was used, to ensure that a build
from a later child commit will always be considered a higher version
number.

Because the version number increases each time the release version
increases, counting only the commits since the last release tag is
sufficient.
@rimashah25 rimashah25 self-requested a review January 10, 2024 17:19
@ocket8888
Copy link
Contributor

It looks like either the Makefile changes aren't matching up with the changes elsewhere for some reason, or possibly there's a place where it needs to be changed that's currently being missed.

cp: cannot stat '../../dist/trafficcontrol-cache-config-8.1.0-.5310b010.el8.x86_64.rpm': No such file or directory

Alternatively, RPM package archive files are totally capable of containing this information - does that really need to be in the filename, too? I know I've said this before, but it seems very redundant to me.

@zrhoffman
Copy link
Member Author

It looks like either the Makefile changes aren't matching up with the changes elsewhere for some reason, or possibly there's a place where it needs to be changed that's currently being missed.

cp: cannot stat '../../dist/trafficcontrol-cache-config-8.1.0-.5310b010.el8.x86_64.rpm': No such file or directory

Fixed in f49cf08 and 2262faf

Alternatively, RPM package archive files are totally capable of containing this information - does that really need to be in the filename, too?

Yes so that RPMs are sorted in directories, first by version, then by release number.

I know I've said this before, but it seems very redundant to me.

Yes it is, but this PR isn't meant to change that. That's probably a bigger conversation that might get pushback from some people.

@ocket8888
Copy link
Contributor

I checked the build artifacts for the checks on this branch - specifically Traffic Ops - and found that it produced

  • traffic_ops-8.1.0-0.d5e35540.el8.src.rpm
  • traffic_ops-8.1.0-0.d5e35540.el8.x86_64.rpm

so it looks like that's saying there have been zero commits since 8.1.0. That's incorrect not only because 8.1.0 doesn't technically exist (:P) but because this PR consists of a non-zero amount of commits.

tl;dr we got a number now, but it's not the right number.

@zrhoffman
Copy link
Member Author

so it looks like that's saying there have been zero commits since 8.1.0. That's incorrect not only because 8.1.0 doesn't technically exist (:P)

That's the version in VERSION

but because this PR consists of a non-zero amount of commits.

tl;dr we got a number now, but it's not the right number.

The GitHub Actions only clone the repo with a fetch-depth of 5 commits. Because no tags matching the pattern are found within the 5 commits, it defaults to 0. So 0 is expected, in this case.

@ocket8888
Copy link
Contributor

So was that always broken? Do we care if the build artifacts match what a user would build?

@zrhoffman
Copy link
Member Author

So was that always broken?

Yes it was always broken, RPMs from CI outside this PR say include 5 as the commit count

Do we care if the build artifacts match what a user would build?

nah

@ocket8888 ocket8888 merged commit 21d7c9a into apache:master Jan 11, 2024
22 checks passed
rimashah25
rimashah25 reviewed Jan 11, 2024
View reviewed changes
Copy link
Contributor

@rimashah25 rimashah25 left a comment

Choose a reason for hiding this comment

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

The changes and explanation LGTM.

rimashah25 pushed a commit that referenced this pull request Jan 24, 2024
@zrhoffman @rimashah25
Include commits since last release in RPM build number (#7896)
3878228 
* Use number of commits since last release in build number

Previously, the number of commits ever was used, to ensure that a build
from a later child commit will always be considered a higher version
number.

Because the version number increases each time the release version
increases, counting only the commits since the last release tag is
sufficient.

* Set pipefail and escape $() in Makefile

* Include --long to always get relative commit count

* Use pipefail shell option within bash

* Fall back to commit count of 0 in TPv2

(cherry picked from commit 21d7c9a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rimashah25 rimashah25 rimashah25 left review comments

@ocket8888 ocket8888 ocket8888 approved these changes

Assignees
No one assigned
Labels
build related to the build process cdn-in-a-box related to the Docker-based CDN-in-a-Box system documentation related to documentation Traffic Portal v2 Related to the experimental Traffic Portal version 2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zrhoffman @ocket8888 @rimashah25