Always load the config file using the API #148
Conversation
| @@ -142,28 +126,12 @@ test("load non-local input with invalid repo syntax", async t => { | |||
| }); | |||
| }); | |||
|
|
|||
| test("load non-existent input", async t => { | |||
There was a problem hiding this comment.
I note we don't actually have a test for when a remote config file doesn't exist. Instead of deleting this test case could you convert it to the case where the file doesn't exist, and check that the error message it gives is acceptable?
| const configFilePath = configFile.substr(2); | ||
| const remote = util.getRequiredEnvParam("GITHUB_REPOSITORY") + "/" + configFilePath | ||
| + "@" + util.getRef(); | ||
| parsedYAML = await getRemoteConfig(remote); |
There was a problem hiding this comment.
I find it odd that we're constructing the remote here in the repo/path@ref format just so we can parse the various bits out of it again. I'm worried this just risks exaccerbating errors in the parsing code. Would it make sense to instead change getRemoteConfig to take the repository, path, and ref as separate arguments, then move the code for parsing user input into those three pieces to the else branch here?
| // For using the api we have to remove the starting "./" | ||
| const configFilePath = configFile.substr(2); | ||
| const remote = util.getRequiredEnvParam("GITHUB_REPOSITORY") + "/" + configFilePath | ||
| + "@" + util.getRef(); |
There was a problem hiding this comment.
We could be slightly more specific and use the commit SHA instead of the ref. The advantage this would give is that it'll be more repeatable if the workflow is retried. It'll always use the commit the workflow was triggered on instead of using the latest version and potentially changing behaviour.
|
This PR would allow us to improve setup by migrating towards a state where users would only need to insert a single action into their workflows. However it would involve making an extra API call, which may put some users over the API limit. Since we haven't observed many problems with workflow setup, but we have observed problems with users going over the API limit, we don't plan to pick up this work in the foreseeable future, so I'll close this PR. |
This PR is a prerequisite for using the prehooks. As we want to have the init step as a prehook, and that would happen before the checkout, we cannot rely on looking for the file locally.
Merge / deployment checklist