Emit NDJSON git-tag events when CHANGESETS_OUTPUT is set - #2129
Conversation
Sorry, something went wrong.
🦋 Changeset detectedLatest commit: d37140a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #2129 +/- ##
==========================================
+ Coverage 87.45% 87.58% +0.13%
==========================================
Files 71 72 +1
Lines 2582 2602 +20
Branches 690 693 +3
==========================================
+ Hits 2258 2279 +21
+ Misses 300 299 -1
Partials 24 24 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
| } | ||
|
|
||
| if (outputPath && process.env.CHANGESETS_OUTPUT_PATH) { | ||
| options.outputPath = process.env.CHANGESETS_OUTPUT_PATH; |
There was a problem hiding this comment.
2 problems here...
- we support --output + --out-dir (in different commands). It feels to me we should standardize on --out-file + --out-dir and then rename this variable to CHANGESETS_OUT_FILE
- all pre-existing --outputs spit out JSON. I like the idea of using NDJSON for those git-tag events. It seems just much easier to manage and extend. This would kinda mean we should refactor existing --output to also use the NDJSON format if we want to follow this path. This would also affect the recently added publish-plan (where a single JSON is kinda easier to manage but given we are primary producers/consumers of that publish plan... we can take it)
Sorry, something went wrong.
There was a problem hiding this comment.
Although, after the second consideration... it wouldn't be good if publish-plan would return NDJSON. Or rather, returning that is fine... but we feed that plan back to changeset pack and it feels weird to make that accept NDJSON file. Or maybe not?
Sorry, something went wrong.
There was a problem hiding this comment.
i think using different formats is fine.
Sorry, something went wrong.
There was a problem hiding this comment.
- I don't think it should be --out-file and --out-dir. Both are for different things: output logs and command-specific artifact directory
- I think mixing JSON and NDJSON is fine too. We can keep a rule where long-running tasks or "potential to fail mid-process but mid-output logs is still important" to use NDJSON, and the rest to use JSON? Then that justifies the split we have now.
Sorry, something went wrong.
There was a problem hiding this comment.
Ok, let's roll with the split logic - where some commands use NDJSON and some use JSON (a single JSON output is still valid NDJSON anyway 😉 ).
I don't think it should be --out-file and --out-dir. Both are for different things: output logs and command-specific artifact directory
What names do you suggest sticking to then?
Sorry, something went wrong.
There was a problem hiding this comment.
I think the current --output and --out-dir is fine.
Maybe if I'd start from scratch, I wouldn't do --output and support --json only (outputs to stdout) so it's more natural for script piping. It can be used for both json and ndjson output (or --ndjson if we really want to). And support an CHANGESETS_JSON_LOG_PATH env var that also allows writing the logs to a specific file (only env var and never a flag, so it works like debug logging). But that's just perhaps a different way to achieve the same thing and not worth it.
Sorry, something went wrong.
There was a problem hiding this comment.
I think it is still worth supporting those output files with a single env variable.
I have just pushed support for this for changeset status and changeset publish-plan so a more full picture could be reviewed
Sorry, something went wrong.
|
The new changes also work for me. Is it too far to shorten the env var to CHANGESETS_OUTPUT to match --output? (Since it's not --output-file) |
Sorry, something went wrong.
This is an alternative to the clunky system we were relying on until now. We had to temporarily bring back the old thing recently as it was removed because it was just a totally implicit way of handling things (see this)
This will pair with changesets/action#678