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

chore: Improve sample apps build info #458

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mahmoud-elmorabea
Copy link
Contributor

@mahmoud-elmorabea mahmoud-elmorabea commented Oct 25, 2024

Motivation

This PR updates the build info we show on sample apps. The changes aims to make those build info useful to the development team and other stakeholders determining how the sample app they are using was built. It also helps the development team reproduce the same build when they get issues or reports.

The new build info

SDK version

This is the version of the SDK that the sample app is using, it can have the following values:

  • A version "x.y.z" when a released SDK version was used
  • "as source code (branch name)" when the SDK was built as source code from a certain branch
  • "as source code (local development)" when it was built locally on a developer's machine

Build date

The date and time when the sample app was built, displayed in local time zone of the user

Branch

The branch from which the sample app was built

Default workspace

The default workspace that the app connects to by default or when resetting defaults in the settings screen

App version

This is unique version code of the sample app to identify the specific app version

Screenshots

Here's a screenshot of different cases and how the info is displayed.

Local with local SDK Local with SDK version CI with local SDK CI with SDK version

@mahmoud-elmorabea mahmoud-elmorabea self-assigned this Oct 25, 2024
Copy link

github-actions bot commented Oct 25, 2024

Sample app builds 📱

Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.


Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.86%. Comparing base (8b30804) to head (2642e7e).
Report is 14 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #458      +/-   ##
============================================
+ Coverage     41.98%   51.86%   +9.88%     
- Complexity      259      291      +32     
============================================
  Files            99       96       -3     
  Lines          2320     2570     +250     
  Branches        344      352       +8     
============================================
+ Hits            974     1333     +359     
+ Misses         1247     1133     -114     
- Partials         99      104       +5     

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

Copy link

Build available to test
Version: MBL-607-improve-build-info-SNAPSHOT
Repository: https://s01.oss.sonatype.org/content/repositories/snapshots/

Copy link

github-actions bot commented Oct 25, 2024

📏 SDK Binary Size Comparison Report

No changes detected in SDK binary size ✅

@mahmoud-elmorabea mahmoud-elmorabea force-pushed the MBL-607-improve-build-info branch 2 times, most recently from d71bbb7 to 90321af Compare October 27, 2024 12:10
@mahmoud-elmorabea mahmoud-elmorabea marked this pull request as ready for review October 27, 2024 12:29
@mahmoud-elmorabea mahmoud-elmorabea requested a review from a team October 27, 2024 12:29
"SDK version: %s\n" +
"Build date: %s\n" +
"Branch: %s\n" +
"Default workspace: Native iOS & Android\n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably rather have that data being injected via secrets than hardcode? cause the next time we wish to change source, we can do that with CI but we will need to do another release just to change this hardcoded string.

or skip it if we don't want to have it via secret?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point about trying to make the value dynamic.

However, the CI values (if it's secrets of env variables) are picked up when you build a new samples app version. So even if it's done using secrets, you still need another release to pick the new values, they won't change for existing builds.

Also I don't see those default workspaces changing too often.

If you are okay with it, let's KISS this one and the first time we have to change it, we make it dynamic 😄

@scotttwittrockcio
Copy link

SDK Version:
I don't think as source code is going to provide much information to users of the app. And including the branch name is duplicate information. Can we set the version to the latest release and the current commit so 3.0.0-b92a8c1? Having the branch name in the SDK version might also cause issue with the user agent parsing, specifically the slash.

Branch:
We should include the commit hash at the end of the branch name. So it would look something like feature/cdp-sdk-b92a8c1. This has been helpful in the past to ensure the test app matches the exact commit when testing.

One thing that I missed in the initial spec is what we set the app version to. Since it shows up in fly, can we set that to the branch? This would be where it shows "main" in the screenshot. This would make it really easy to quickly see what branch was being tested when looking in fly.
image

@mahmoud-elmorabea mahmoud-elmorabea requested review from Shahroz16 and a team October 30, 2024 00:59
@mahmoud-elmorabea
Copy link
Contributor Author

Hey @scotttwittrockcio , great feedback! Thank you.

Let me break it down.

SDK Version:
I don't think as source code is going to provide much information to users of the app. And including the branch name is duplicate information. Can we set the version to the latest release and the current commit so 3.0.0-b92a8c1? Having the branch name in the SDK version might also cause issue with the user agent parsing, specifically the slash.

  • Technically, users who aren't in our team shouldn't see the local and as source code since those only show when you build the code and run it locally. This does help our team to differentiate easily which build we are looking at and which SDK is it pointing to.
  • I can remove the branch name from the SDK version part since that is indeed redundant.
  • We don't actually set the SDK version to the value you see in the sample app, this is just a visual we construct for the sample app. The SDK version that is sent to our server is either the branch name or the real version if that is used.

Branch:
We should include the commit hash at the end of the branch name. So it would look something like feature/cdp-sdk-b92a8c1. This has been helpful in the past to ensure the test app matches the exact commit when testing.

Sure, we can do that.

One thing that I missed in the initial spec is what we set the app version to. Since it shows up in fly, can we set that to the branch? This would be where it shows "main" in the screenshot. This would make it really easy to quickly see what branch was being tested when looking in fly.

The app version is not changed from what it is now.

@mahmoud-elmorabea
Copy link
Contributor Author

@scotttwittrockcio , @Shahroz16 I've updated the PR, please take a look at the updated screenshots.

Copy link
Contributor

@Shahroz16 Shahroz16 left a comment

Choose a reason for hiding this comment

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

Given Scotts changes are done too, and we can revisit the workspace later, its good to go

Copy link

  • java_layout: MBL-607-improve-build-info (1730284877)

Copy link

  • kotlin_compose: MBL-607-improve-build-info (1730284882)

@scotttwittrockcio
Copy link

scotttwittrockcio commented Oct 30, 2024

Technically, users who aren't in our team shouldn't see the local and as source code since those only show when you build the code and run it locally

If I understand this correctly, the CI with local SDK group will still get distributed. Some TS users will also subscribe to the next and feature groups. Can we just rename this to development build as that is more clear than as source code?

The SDK version that is sent to our server is either the branch name or the real version if that is used.

Can we update this to replace an '/' with '.' in the sdk version that is sent to the server? The slash can break our parsing of the user agent string.

@scotttwittrockcio
Copy link

scotttwittrockcio commented Oct 30, 2024

Output from our chat.
For the SDK version and branch name this was the format we landed on feature.cdp-sdk.b92a8c1. If it's easier to replace the / anywhere we reference the branch name, that's great. The place it really matters is in the user agent, as having the / in the breaks the parsing we do for that value.

Theres a follow up ticket to make this happen here: https://linear.app/customerio/issue/MBL-647/update-sdk-version-in-builds

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.

3 participants