Emit NDJSON git-tag events when CHANGESETS_OUTPUT is set - #2129
Conversation
🦋 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ 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. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| 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-dirand then rename this variable toCHANGESETS_OUT_FILE - all pre-existing
--outputs spit out JSON. I like the idea of using NDJSON for thosegit-tagevents. It seems just much easier to manage and extend. This would kinda mean we should refactor existing--outputto also use the NDJSON format if we want to follow this path. This would also affect the recently addedpublish-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)
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?
There was a problem hiding this comment.
i think using different formats is fine.
There was a problem hiding this comment.
- I don't think it should be
--out-fileand--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.
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?
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.
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
git-tag events when CHANGESETS_OUTPUT_PATH is setgit-tag events when CHANGESETS_OUTPUT_FILE is set
|
The new changes also work for me. Is it too far to shorten the env var to |
git-tag events when CHANGESETS_OUTPUT_FILE is setgit-tag events when CHANGESETS_OUTPUT is set
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