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.

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

Supported configuration file extension check does not use the per-generator default #104

Closed
haykam821 opened this issue Sep 4, 2023 · 6 comments · Fixed by #139
Closed
Assignees

Comments

@haykam821
Copy link

The setPagesConfig function uses different error messages depending on whether the generator configuration file has a correct extension. However, the configuration file used for this check is based on the context generator_config_file input only, rather than the default configuration file for the generator used that is computed by the getConfigParserSettings function.

As a result, if generator_config_file is not specified, then errors within the setPagesConfig function will always be blamed on an incorrect file extension.

@janbrasna
Copy link

Basically the check:

} catch (error) {
const isSupportedFileExtension = SUPPORTED_FILE_EXTENSIONS.some(ext => generatorConfigFile.endsWith(ext))
// Logging
if (!isSupportedFileExtension) {
core.warning(
`Unsupported configuration file extension. Currently supported extensions: ${SUPPORTED_FILE_EXTENSIONS.map(
ext => JSON.stringify(ext)
).join(', ')}`,
error
)

should return isSupportedFileExtension true if !generatorConfigFile and that's all I guess.

(It would be better to use settings.configurationFile output instead that should have the template config path filled in if no passed config, but that's another scope and the actual getter might have thrown so no point getting the output again, oh well…)

So maybe just something like:

--- a/src/set-pages-config.js
+++ b/src/set-pages-config.js
@@ -76,12 +78,12 @@ function setPagesConfig({ staticSiteGenerator, generatorConfigFile, siteUrl }) {
     const settings = getConfigParserSettings({ staticSiteGenerator, generatorConfigFile, siteUrl })
     new ConfigParser(settings).injectAll()
   } catch (error) {
-    const isSupportedFileExtension = SUPPORTED_FILE_EXTENSIONS.some(ext => generatorConfigFile.endsWith(ext))
+    const isSupportedFileExtension = !generatorConfigFile || SUPPORTED_FILE_EXTENSIONS.some(ext => generatorConfigFile.endsWith(ext))
 
     // Logging
     if (!isSupportedFileExtension) {

@JamesMGreene
Copy link
Contributor

I believe the main problem here (our failure to log the actual error message) is resolved by:

As for failure to correctly detect config files, I'm going to leave that in another issue:

@janbrasna
Copy link

@JamesMGreene Thanks for keeping this in mind. The issue here is different than the referenced autodetection. To quote the important bit:

" if generator_config_file is not specified, then errors within the setPagesConfig function will always be blamed on an incorrect file extension."

To simplify, if you don't pass any config file and the action uses its own implicit boilerplate, the conditional logic testing whether to log "We were unable to determine…" or "Unsupported configuration file extension…" fails because

const isSupportedFileExtension = SUPPORTED_FILE_EXTENSIONS.some(ext => generatorConfigFile.endsWith(ext))
this doesn't work for when there's no config specified, as generatorConfigFile.endsWith(ext) is empty/falsy, so it "seems" that all errors are then blamed on extension, when that isn't the case if you're not providing a config at all;)

It's just cosmetics though.

@JamesMGreene
Copy link
Contributor

Ah, sorry, thanks for persevering on explaining this one! 😅 I see what you mean now. 👌

@janbrasna
Copy link

No worries;) I'd have already demonstrated it better by proposing a fix just by adding the const isSupportedFileExtension = !generatorConfigFile || SUPPORTED_FILE_EXTENSIONS.some… to avoid blaming any errors without config file present on its extension straight away, however TBH I lack the imagination to properly test such change esp. in edge case error handler and not to introduce any potential surface for more issues stemming from it so I leave that to others… I see you've taken more systematic approach to the whole configs magic so I'm just watching from the passenger seat: Good job! 🎩

@JamesMGreene
Copy link
Contributor

A fix for this is now included in actions/configure-pages@v5. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants