Add fileSystemStoragePath #61
Conversation
denar90
force-pushed
the
feature/output-manifest
branch
from
7569d4b to
4858399
Compare
|
|
||
| core.setOutput('resultsPath', resultsPath) | ||
| core.setOutput('links', links ? JSON.stringify(links) : '') | ||
| core.setOutput('assertionResults', assertionResults ? JSON.stringify(assertionResults) : '') | ||
| core.setOutput('manifest', manifestResults ? JSON.stringify(manifestResults) : '') |
There was a problem hiding this comment.
or do we prefer reports instead?
There was a problem hiding this comment.
I think, it's better to match LHCI namings.
action.yml
Outdated
| @@ -17,6 +17,8 @@ inputs: | |||
| description: 'Address of a LHCI server' | |||
| serverToken: | |||
| description: 'API token to push to LHCI server' | |||
| fileSystemStoragePath: | |||
There was a problem hiding this comment.
Why do we need this option? The data is already stored in the file system.
If it's only to generate manifest.json and expose it with output, we could run it automatically.
There was a problem hiding this comment.
If I'm not mistaken, manifest.json is generated for the outputdir option (doc)
Also it's covered only by this part of code
@patrickhulce what do you think about generating manifest.json for any target, not only filesystem?
There was a problem hiding this comment.
what do you think about generating manifest.json for any target, not only filesystem?
I think it doesn't make as much sense since the point was to point to the reports on disk. There's nothing inherent to LHCI that means you couldn't just run upload multiple times. I'd probably consider supporting multiple targets before I'd consider writing manifest.json for non filesystem targets. Would that help? It'd just be a workaround for running upload more than once on your own though
There was a problem hiding this comment.
From my perspective I found manifest summary really useful. I don't need to aggregate results or something, just passing manifest data to other action like:
- name: Audit URLs using Lighthouse
id: LHCIAction
uses: treosh/lighthouse-ci-action@v3
with:
urls: |
https://example.com
uploadArtifacts: true # save results as an action artifacts
temporaryPublicStorage: true # upload lighthouse report to the temporary storage
- name: Webhook
uses: denar90/webhook-action@master
env:
webhookUrl: ${{secrets.WEBHOOK_URL}}
data: '{ "links": ${{steps.LHCIAction.outputs.links}}, "manifest": ${{steps.LHCIAction.outputs.manifest}} }'On other hand it's not need to run filesystem target when reports needed to be uploaded. But maybe it's state of thinking for my personal perspective and having just one more round of uploading (uploading to filesystem) is ok :)
There was a problem hiding this comment.
That's true there's some definite value in the summary and I suppose the manifest could point to the link of the report instead of path on disk. Do you have a proposal for where the manifest would be written for non-filesystem targets?
I guess perhaps a new option with a path for it? How does that interplay with the filesystem output dir? What if they're conflicting? etc
There was a problem hiding this comment.
I'd say let's keep manifest.json inside output dir - .lighthouseci.
In case outputDir is set up, manifest.json can be stored inside provided path for the outputDir. If manifest files are conflicting, last one - wins.
My suggestion is simple, but I'm not sure it feats all the cases of LHCI users.
There was a problem hiding this comment.
@patrickhulce one more. can you help me understand why can't we have manifest.json generated no matter what target is?
Is it because we need to address:
I guess perhaps a new option with a path for it? How does that interplay with the filesystem output dir? What if they're conflicting? etc
?
There was a problem hiding this comment.
manifest.json behavior as it stands right now is part of the public API defined by the target=filesystem options.
I'm happy to write some version of the manifest with the paths on disk missing to .lighthouseci folder for your internal use but not expecting it to respect semver.
To turn it into a formal API that's outside of internal .lighthouseci requires solving all those problems I mentioned.
There was a problem hiding this comment.
thank you 👍
so, inside action we can write or use from LHCI the same helper function with logic stand by for composing all needed data for manifest.json
|
|
||
| core.setOutput('resultsPath', resultsPath) | ||
| core.setOutput('links', links ? JSON.stringify(links) : '') | ||
| core.setOutput('assertionResults', assertionResults ? JSON.stringify(assertionResults) : '') | ||
| core.setOutput('manifest', manifestResults ? JSON.stringify(manifestResults) : '') |
There was a problem hiding this comment.
I think, it's better to match LHCI namings.
README.md
Outdated
| @@ -394,6 +394,37 @@ Add a plugin as a dependency, so it's installed locally: | |||
|
|
|||
| </details> | |||
|
|
|||
| <details> | |||
| <summary>Output usage</summary><br> | |||
There was a problem hiding this comment.
Looks boring :) any ideas on better title ?
There was a problem hiding this comment.
Use output for a powerful composition with other actions
README.md
Outdated
| @@ -394,6 +394,37 @@ Add a plugin as a dependency, so it's installed locally: | |||
|
|
|||
| </details> | |||
|
|
|||
| <details> | |||
| <summary>Output usage</summary><br> | |||
There was a problem hiding this comment.
Use output for a powerful composition with other actions
|
👋 Hello! Is there any way we can get this merged and a new version out the door? Looking forward to the features in this PR 🙌 |
|
Soon should be shipped to #v3 tag with a few minor changes. |
Solves #41