RUM-7285 Add api-surface step to Lint stage #2112
Conversation
mariedm
force-pushed
the
rum-7285-add-api-surface-step-to-ci
branch
6 times, most recently
from
510cd2c to
7626f46
Compare
Datadog ReportBranch report: ✅ 0 Failed, 3570 Passed, 0 Skipped, 2m 19.07s Total Time 🔻 Code Coverage Decreases vs Default Branch (4) |
.gitlab-ci.yml
Outdated
| - if ! diff api-surface-swift api-surface-swift-generated; then | ||
| echo "❌ Swift API surface mismatch"; | ||
| echo "👉 Run \`make api-surface\` locally to update \`api-surface-swift\` and commit the changes."; | ||
| exit 1; | ||
| fi | ||
| - if ! diff api-surface-objc api-surface-objc-generated; then | ||
| echo "❌ Objective-C API surface mismatch"; | ||
| echo "👉 Run \`make api-surface\` locally to update \`api-surface-objc\` and commit the changes."; | ||
| exit 1; | ||
| fi |
There was a problem hiding this comment.
suggestion/ Could we move these checks to separate script in tools/ directory? The convention we started for .gitlab-ci.yml is to avoid bare scripts in yml as they are very inconvenient to reproduce in local shell.
Ideally, we could get inspired by make rum-models-verfy:
make api-surface- to generate API surface files; ran in localmake api-surface-verify- to verify API surface; ran on CI
Makefile
Outdated
| SWIFT_OUTPUT_PATH := api-surface-swift-generated | ||
| OBJC_OUTPUT_PATH := api-surface-objc-generated | ||
| endif | ||
|
|
||
| # Generate api-surface files for Datadog and DatadogObjc. | ||
| api-surface: | ||
| @echo "Generating api-surface-swift" |
There was a problem hiding this comment.
suggestion/ Let's follow the convention in of printing command's title before it is ran. This brings a lot of clarity to CI logs:
@$(ECHO_TITLE) "make api-surface"
Also, let's fix the indentation for this command. It uses the old one, all new commands (after migration to GitLab) are indented with less whitespaces. See these examples.
mariedm
force-pushed
the
rum-7285-add-api-surface-step-to-ci
branch
from
7626f46 to
4aa62a9
Compare
|
Update
|
There was a problem hiding this comment.
Well done 🏅💪!
I did notice however, that integrating api-surface has increased the lint job duration from 30 seconds to around 3 minutes. This is a significant change, so before merging, it might be worth discussing with the team:
- How do we feel about this increased duration?
- How might this impact development or the feedback loop?
- Are there any optimization opportunities?
- When should we consider optimizing?
| // Given | ||
| let command = try SPMLibrarySurfaceCommand.parse([ | ||
| let temporaryFile = "/tmp/api-surface-test-output" |
There was a problem hiding this comment.
suggestion/ Using a dynamically created temp directory could bring some benefits:
- Avoid conflicts between test runs and ensure isolation.
- Automatically clean up after tests to prevent leftover files from affecting others.
- More robust (in other environments) than using hardcoded paths.
Example:
let tempDirectory = FileManager.default.temporaryDirectory.appendingPathComponent("com.datadoghq.api-surface-" + UUID().uuidString)
try FileManager.default.createDirectory(at: tempDirectory, withIntermediateDirectories: true, attributes: nil)
let temporaryFile = tempDirectory.appendingPathComponent("api-surface-test-output").path
What and why?
To ensure public API changes are properly updated, I previously added a reminder in our GitHub PR template checklist to run
make api-surface. However, this has proven insufficient 🙃 To address this, I am adding an automated step to the CI Lint phase to enforce this check.How?
make api-surface.api-surface-swift-generatedandapi-surface-objc-generated).api-surface-swiftandapi-surface-objc).make api-surfacelocally and commit the updated reference files.Review checklist
make api-surface)