Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve error messages for external config files #330

Merged
merged 11 commits into from Nov 16, 2022
Merged

Improve error messages for external config files #330

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0a055a6
Improve error messages for external config files.
febuiles Nov 15, 2022
c2097b2
Updating copy in a test.
febuiles Nov 15, 2022
c57c602
Force error casting to get messages!
febuiles Nov 15, 2022
2d265aa
Updating dist.
febuiles Nov 15, 2022
1dd7392
Adding fixture for testing config file string lists.
febuiles Nov 15, 2022
ba127ca
Adding a test to confirm lists work properly in config files.
febuiles Nov 15, 2022
93652d7
Fix failing tests.
febuiles Nov 15, 2022
ac059c6
Checkpoint!
febuiles Nov 15, 2022
b8e622f
Move test out of failing block.
febuiles Nov 15, 2022
a952d7b
adding dist
febuiles Nov 15, 2022
369356e
Fixing merge conflict in dist/
febuiles Nov 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion __tests__/config.test.ts
View file
Open in desktop
Expand Up @@ -113,7 +113,7 @@ test('it reads an external config file', async () => {

test('raises an error when the the config file was not found', async () => {
setInput('config-file', 'fixtures/i-dont-exist')
await expect(readConfig()).rejects.toThrow(/Unable to fetch config file/)
await expect(readConfig()).rejects.toThrow(/Unable to fetch/)
})

test('it parses options from both sources', async () => {
Expand Down
22 changes: 18 additions & 4 deletions dist/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/index.js.map
View file
Open in desktop

Large diffs are not rendered by default.

21 changes: 17 additions & 4 deletions src/config.ts
View file
Open in desktop
Expand Up @@ -80,7 +80,13 @@ function validateLicenses(
if (licenses === undefined) {
return
}
const invalid_licenses = licenses.filter(license => !isSPDXValid(license))

let invalid_licenses
try {
invalid_licenses = licenses.filter(license => !isSPDXValid(license))
} catch (error) {
throw new Error(`Error validating license(s): ${(error as Error).message}`)
}

if (invalid_licenses.length > 0) {
throw new Error(
Expand Down Expand Up @@ -113,8 +119,8 @@ async function readConfigFile(
}
return parseConfigFile(data)
} catch (error) {
core.debug(error as string)
throw new Error('Unable to fetch config file')
core.debug((error as Error).message)
throw new Error('Unable to fetch or parse config file')
}
}

Expand All @@ -123,7 +129,14 @@ function parseConfigFile(configData: string): ConfigurationOptionsPartial {
const data = YAML.parse(configData)
for (const key of Object.keys(data)) {
if (key === 'allow-licenses' || key === 'deny-licenses') {
validateLicenses(key, data[key])
const licenses = data[key]
// handle the case where the user has a singe or invalid license
// in the config file
if (typeof licenses === 'string') {
validateLicenses(key, [licenses])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intended to treat the whole provided value as one license? This might be confusing in the case of the reported issue. Or maybe we should just throw an error for non list values provided 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original comment had this structure for an external config file:

allow-licenses: FOO, BAR, BAZ, BLA

In YAML that is equal to allow-licenses: "FOO, BAR, BAZ, BLA" which is indeed one license.

I think the confusion stems from the fact that in the inline configuration you can provide a list by using commas (GitHub Actions don't support array-like inputs), while the external config file expect YAML lists.

Now that you bring this up I'm noticing that I'm likely blowing up our inline license parsing with this change and I need to make some changes, right? 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd thought bit would only impact the config file parsing and yes, now I see the source of confusion you pointed out

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the confusion stems from the fact that in the inline configuration you can provide a list by using commas (GitHub Actions don't support array-like inputs), while the external config file expect YAML lists.

@febuiles I think that's definitely a problem; we should not have a discrepancy between the inline configuration and the external configuration. Is there anything stopping us from parsing the external configuration in the same way as we parse the inline configuration, while also allowing YAML arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hmaurer I just did a quick read of the SPDX license names guidelines and they specifically ask not to use commas in the full name/ID. I think supporting the same format with commas for both config options is what makes sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This went from being a "Fix an error message" PR to "Fix lists and also an error messages". I've updated the PR description and cleaned up the code a bit to account for the cases where we have comma-separated lists in external configs.

} else {
validateLicenses(key, data[key])
}
}
// get rid of the ugly dashes from the actions conventions
if (key.includes('-')) {
Expand Down