Add RPM version string substitutions to only allow valid characters. #1438
Conversation
- In DEB builds, the DEB version is defined as PKG_VERSION~debian_release, and CODE_VERSION strips everything after the "~", so it's the same as PKG_VERSION. - In RPM builds, the RPM version is defined as PKG_VERSION and CODE_VERSION is set to the RPM version. This partially undoes the changes from #69.
c91b333 to
000d018
Compare
pkg/rpm/google-cloud-ops-agent.spec
Outdated
There was a problem hiding this comment.
Why are you eliminating this logic? This is to make sure that the RPM version gets passed into build.sh; without this it's possible that the RPM version and the version that Ops Agent reports at runtime with will differ.
There was a problem hiding this comment.
See the commit message of 3328aa6. Previously, CODE_VERSION was always the same as PKG_VERSION in our automated builds, so it was a no-op from the automated build standpoint. With this PR, the RPM version is actually a transformed PKG_VERSION, which makes the CODE_VERSION inconsistent between RPM and DEB builds.
If you want to override the version in manual builds, you could just edit PKG_VERSION, right? Can you please give an example of a use case where you'd want the RPM package version (and thus the version reported in the agent metrics) to differ from PKG_VERSION?
There was a problem hiding this comment.
The package version will already be different from the Ops Agent version; distros sometimes add various prefixes when building packages (such as ~focal). I guess the question is whether we want metrics to report the final package version or the underlying source version. IMO we should report the package version. Also it just feels wrong to have two different layers of the stack independently determine the version number - it's supposed to be the package manager's job to track the version number.
There was a problem hiding this comment.
The logic I ripped out actually stripped off those extra suffixes. So, regardless of my opinion below, this PR is preserving the original behavior for numeric versions. The only question is whether we should allow overriding the PKG_VERSION variable at build time.
I believe that the version compiled into the agent itself really ought to be the agent version, not the package version. Other tools (like OS inventory manager) should be available to report the package versions. Looking at it another way, the metrics ought to report the version of the code, not the package.
There was a problem hiding this comment.
I guess I'm thinking about cases where the package patches the code. I don't think it's necessarily a valid assumption that the code is the same across all package builds. But I think this is a nit, and we can always change this in the future, so shrug.
60b9bb1 to
b6b00b1
Compare
…tutions." This reverts commit 000d018.
This undoes an accidental change from #1438, and makes the version string on Debian platforms use the codename again.
Description
Add version string substitutions when building RPM packages to only allow valid characters.
According to the Fedora Packaging Guildelines, only "ASCII letters (upper and lower case), digits and periods" are allowed in RPM version strings.
Related issue
b/267347168
How has this been tested?
Temporarily pushed a commit with an invalid RPM version string in
VERSION, and observed that the automated builds succeeded. Without these changes, the build failed with:Checklist: