RUMM-2244: Sourcemap upload script by 0xnm · Pull Request #222 · DataDog/dd-sdk-reactnative

0xnm · 2022-06-23T12:38:43Z

What does this PR do?

This changed brings a task to upload JS source maps/bundles for Android by getting their location and passing it to the datadog-ci tool. Currently it only supports application module (hence relying on the specific project) and should be applied as apply from in the end of the app module's build.gradle.

It can read properties from the datadog-sourcemaps.properties file (currently reading only service name property + API key), which should be located in the root dir of React Native project. Content should be in the key=value format.

For the custom service name the logic is to check if there is SERVICE_NAME_ANDROID_%FLAVOR% key first, if not check SERVICE_NAME_ANDROID, otherwise fallback to applicationId.

When applied it will add the necessary upload tasks for the variants which have bundling enabled. This task will be automatically run after bundle task completes.

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)
If this PR is auto-generated, please make sure also to manually update the code related to the change

louiszawadzki · 2022-06-23T12:52:15Z

+    }
+
+    // TODO service name per variant?
+    def serviceName = datadogConfig.SERVICE_NAME ?: androidExtension.defaultConfig.applicationId


Question: should we be able to provide this as an environment variable to get a similar behaviour on iOS and Android?

Can you please point me how it is defined/used in case of iOS?

for iOS I thought we could specify it in the Build phase. But now I feel this is a bit too hard to implement.
Now there is an .xcode.env file in the ios folder with env variables that are going to be used for the build.

I thought we could do a .datadog.sourcemaps.env file at the root of the project, which could contain variables like:

DATADOG_API_KEY

DATADOG_SITE

DATADOG_ANDROID_SERVICE

DATADOG_IOS_SERVICE

This file can be gitignored so the variables could also be specified in the CI without having to create or decrypt a file.

In the plugin, we could try to source it if is present, then use the env variable that is present.
I think there is an advantage of having only 1 file for iOS and Android, and it integrates nicely with the CI, Fastlane or the .xcode.env (we can add a line to source our file from this one).

What do you think?

the only downside is that not everything should be sourced (service name is not read as env variable by datadog-ci, if I'm not wrong?) and there is no source on Windows (but it is not a big deal, with Gradle we can source by ourselves), but overall I'm ok with such common file.

Indeed the service name is not read as an environment variable by datadog-ci, I don't think it should be (if they want to directly use this command, users can create a script that gets this variable and feeds it into the script).

I think the advantage of having 1 config file is that it makes it easy for devs to know where to put things.

louiszawadzki · 2022-06-23T12:55:38Z

+    def serviceName = datadogConfig.SERVICE_NAME ?: androidExtension.defaultConfig.applicationId
+
+    if (serviceName == null) {
+        throw new GradleException("Cannot determine service name.")


I believe a lot of people are going to feel helpless at short messages.
We could improve this message by adding a few troubleshooting actions.
For example:
"Make sure that you've applied the datadog plugin in your android/app/build.gradle (or equivalent) and it has a defaultConfi applicationId."
"You can specify a custom service from the datadog.properties file"

louiszawadzki · 2022-06-23T12:56:13Z

+    def buildVersion = androidExtension.defaultConfig.versionCode
+
+    if (releaseVersion == null) {
+        throw new GradleException("Cannot determine application version," +


Same thing here, I think we should assume that the person reading this message is not familiar with android development.

louiszawadzki · 2022-06-23T13:35:38Z

The file needs to be added to the files in packages/core/package.json, and then the release-content needs to be updated

louiszawadzki · 2022-06-23T14:17:37Z

To test this with a new version of datadog-ci:
In your local datadog-ci

yarn link
yarn build

In your project with a new version of the core package

yarn link @datadog/datadog-ci
copy datadog-ci/dist/cli.js to node_modules/bin/datadog-ci

louiszawadzki · 2022-06-27T13:54:53Z

This PR is good for me, let's wait for the unminification to be working in staging before merging it.

mariusc83 · 2022-07-06T11:36:09Z

+        }
+
+        uploadTask.dependsOn bundleTask
+        bundleTask.finalizedBy uploadTask


why this also ? isn't the first dependency enough ?

No, not enough. Without uploadTask.dependsOn bundleTask there will be an error if user runs upload task, but there is no bundle task run before by some reason. Without bundleTask.finalizedBy uploadTask there won't be automatic upload after bundle task is finished.

I see, tks for the explanation. That makes sense.

mariusc83 · 2022-07-06T11:42:08Z

+    }
+}
+
+def runShellCommand(List<String> command, Map<String, String> env = [:]) {


there's already an Exec gradle task from which you can inherit one. Here's the example of the one I did when working for the nightly tests coverage:

tasks.create("checkNightlyTestsCoverage", Exec::class.java) { this.group = "datadog" val generateApiTasks = allprojects.mapNotNull { it.tasks.findByName("generateApiSurface") } setDependsOn(generateApiTasks) setCommandLine( "python3", "instrumentation-tools/nightly_tests_code_coverage.py", "-t $threshold" ) }

yes, inside runShellCommand there is a call to exec, just the body of the upload task created is responsible for the configuration phase, and actual upload is triggered in the doFirst.

Yup, sorry didn't check your last commit ;)

mariusc83

LGTM

RUMM-2244: Sourcemap upload script

6193593

louiszawadzki reviewed Jun 23, 2022

View reviewed changes

Comment thread packages/core/datadog-sourcemaps.gradle

louiszawadzki reviewed Jun 23, 2022

View reviewed changes

Comment thread packages/core/datadog-sourcemaps.gradle

louiszawadzki reviewed Jun 23, 2022

View reviewed changes

Comment thread packages/core/datadog-sourcemaps.gradle

louiszawadzki reviewed Jun 23, 2022

View reviewed changes

Comment thread packages/core/datadog-sourcemaps.gradle

0xnm added 2 commits June 23, 2022 16:40

Impovements

65bf88d

Flavor-specific service configuration support

569a1ae

0xnm force-pushed the nogorodnikov/rumm-2244/android-sourcemaps-upload-script branch 4 times, most recently from 818a6a9 to 86e8aed Compare June 27, 2022 11:44

Apply sourcemaps script, add test

4bc959e

0xnm force-pushed the nogorodnikov/rumm-2244/android-sourcemaps-upload-script branch from 86e8aed to 4bc959e Compare June 27, 2022 11:49

louiszawadzki added the Do not merge This PR is not ready to be merged yet label Jun 27, 2022

mariusc83 reviewed Jul 6, 2022

View reviewed changes

mariusc83 approved these changes Jul 6, 2022

View reviewed changes

louiszawadzki mentioned this pull request Jul 22, 2022

RUMM 2326, DOCS-3845 Add expo plugin for mapping files #233

Merged

7 tasks

louiszawadzki added 2 commits July 25, 2022 11:00

Remove datadog-sourcemaps config file logic

8a1ea58

Remove API key check to enable datdaog-ci.json config

3b37b62

louiszawadzki removed the Do not merge This PR is not ready to be merged yet label Jul 25, 2022

louiszawadzki marked this pull request as ready for review July 25, 2022 09:22

louiszawadzki requested a review from a team as a code owner July 25, 2022 09:22

0xnm commented Jul 25, 2022

View reviewed changes

Comment thread packages/core/datadog-sourcemaps.gradle Outdated

Print output of sourcemaps upload in gradle logs

454c4a4

louiszawadzki force-pushed the nogorodnikov/rumm-2244/android-sourcemaps-upload-script branch from a3c49f3 to 454c4a4 Compare July 29, 2022 09:50

louiszawadzki merged commit 108acaa into develop Jul 29, 2022

louiszawadzki deleted the nogorodnikov/rumm-2244/android-sourcemaps-upload-script branch July 29, 2022 11:43

Uh oh!

Conversation

0xnm commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Review checklist (to be filled by reviewers)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

louiszawadzki commented Jun 23, 2022

Uh oh!

louiszawadzki commented Jun 23, 2022

Uh oh!

louiszawadzki commented Jun 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariusc83 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

0xnm commented Jun 23, 2022 •

edited

Loading