-
Notifications
You must be signed in to change notification settings - Fork 12
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
Update releaseName
default value
#267
Conversation
releaseName
default value
var releaseName: String { | ||
return Bundle.main.object(forInfoDictionaryKey: kCFBundleVersionKey as String) as! String | ||
let bundleVersion = Bundle.main.infoDictionary?[kCFBundleVersionKey as String] ?? "" | ||
let bundleIdentifier = Bundle.main.infoDictionary?[kCFBundleIdentifierKey as String] ?? "" | ||
let bundleShortVersion = Bundle.main.infoDictionary?["CFBundleShortVersionString"] as? String ?? "" | ||
|
||
// This is the format that Sentry recommends for version numbers. | ||
// See https://docs.sentry.io/platforms/apple/configuration/releases/#bind-the-version | ||
return "\(bundleIdentifier)@\(bundleShortVersion)+\(bundleVersion)" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you feel about adding a unit test to verify the format we compute is as we expect it to be right now and ensure it will remain like that in the future (or, rather, have a safeguard against unintentionally changing it)
@spencertransier I saw CI failed because the change in the source code now requires changes to the tests too, which makes me wonder whether it might require changes in the clients as well. Depending on the amount of work required, and given that the release tracking inconsistency is a blocker for a project in WooCommerce (internal ref pdnsEh-1mT-p2) feel free to ship a version with your override since you already proven it works. We can iterate on this with more clam later. 👍 |
@mokagio When I was chatting with @jkmassel about this the other day, he suggested making Considering that, what do you think about the changes I just pushed to remove the I tried out the latest PR change in the WCiOS test branch and it worked great. The new version showed up in Sentry with the expected format. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that, what do you think about the changes I just pushed to remove the
releaseName
from the protocol entirely and only rely on Sentry's default?
I like the idea!
I tried out the latest PR change in the WCiOS test branch and it worked great. The new version showed up in Sentry with the expected format.
Awesome.
It would be a breaking change and we would need to bump the library to 3.0.0.
Would it be a breaking change? The var { get }
requirement has been removed from the protocol, which makes me think that clients implementing it would not break as the compiler would consider their property declaration to be one for the object itself and not required by the protocol. If we had added one, then that's when I think the compiler would bark at us.
Regardless of the version number, let's do this!
This PR updates the default
releaseName
attribute from sending only theCFBundleVersionKey
to now include theCFBundleIdentifierKey
,CFBundleShortVersionString
, andCFBundleVersionKey
. Sentry releases are global across all projects in an organization, so only relying on theCFBundleVersionKey
was causing conflicts between apps that had the same release number. This will now correctly separate the releases.The
releaseName
format that we're sending now is the default format that Sentry uses for thereleaseName
. I decided that it would be good to explicitly define that format in this library so that we can ensure future consistency for release names across all the apps.I had trouble using the TracksDemo app to test updating the release version in Sentry, so I created a test branch for WooCommerce iOS that points to this dev branch:
test/sentry-release-name
(The only change there is updating thepodfile
to point here)Using that test branch, I was able to verify that the releases are now being sent up to Sentry with the updated attributes. You can see that the
16.2 (16.2.0.0)
and16.1 (16.1.0.1)
releases show the full version information now:CHANGELOG.md
if necessary.