Lint match extension name and directory #42
Conversation
cgraham-rs
left a comment
cgraham-rs
left a comment
There was a problem hiding this comment.
LGTM. I left one comment to with potential improvements we could discuss/consider.
| MANIFEST_NAME=$(jq -r '.extension.name' < ./extensions/${{ inputs.extension-name }}/manifest.json) | ||
| if [ ${{ inputs.extension-name }} != $MANIFEST_NAME ]; then | ||
| echo "Error: Extension name, directory, and manifest.json mismatch" | ||
| echo "Extension '${{ inputs.extension-name }}' must be in the folder '/extensions/${{ inputs.extension-name }}'" and have the name '${{ inputs.extension-name }}' in the manifest.json. |
There was a problem hiding this comment.
I had a little trouble retracing where exactly inputs.extension-name came from. It looks like this is the filter name a human manually typed into the dorny/paths-filter step.
Some potential improvements:
- Be explicit in informing the user the filter as the source of the name in case that is actually where a typo exists. We already have code comments that I think are adequate in educating the user the filter has to match the extension.
- This is extremely nit-picky, is there any areas of improvement for how we refer to these? Referring to extension name in the workflow, but that's really a filter name, and there's also a literal
name:field in the manifest. Minor opportunity for confusion. - Go out of our way to further help the user by providing a verbose diff so it's entirely clear which of the things aren't matching.
There was a problem hiding this comment.
It looks like this is the filter name a human manually typed into the dorny/paths-filter step
This is correct. If the extension name is typo-ed the first line of this will fail saying that ./extensions/${{ inputs.extension-name}}/manifest.json doesn't exist:
MANIFEST_NAME=$(jq -r '.extension.name' < ./extensions/${{ inputs.extension-name }}/manifest.json)
That will be the first clue.
Once that passes then the check is if the manifest has the right name. Those in tandem check the filter, directory, and manifest name all together.
I added improved comments for the filter name in: 4c76cef
I want to avoid commenting in the lint action itself that the name comes from the filter name since that isn't necessarily true for the complex tasks.
There was a problem hiding this comment.
I'm going to get this merged, but we can continue the discussion and follow-up with any other improvements!
dotNomad
force-pushed
the
dotnomad/extension-name-lint
branch
from
fa40e1e to
4c76cef
Compare
This PR adds another step to our extension linting action that checks that the extension directory name and the
extension.namepart of themanifest.jsonmatch.If they do not the linting step fails avoiding issue further on in the workflow.