Detect ubuntu version from /etc/os-release if ImageOS = Linux #327
Conversation
There was a problem hiding this comment.
I'd prefer to keep this in a single file, for now. If the time comes to split we'll do a separate pull request. (also, this way we have to compare/review across different files, which is not ideal)
There was a problem hiding this comment.
Feel free to follow up with a refactor update, if you'd like, but lets keep concerns separated.
| const osVersion = getRunnerOSVersion() | ||
| const osVersion = getRunnerOSVersion(process.env.ImageOS) |
There was a problem hiding this comment.
I'd prefer to keep the process. stuff inside the function.
|
|
||
| core.info(`ImageOS is set to '${imageOS}'`) | ||
|
|
||
| if (imageOS === 'Linux') { |
There was a problem hiding this comment.
Would it be better to just look at /etc/os-release existing than this? Reason for it is we seem to be moving from one constraint to the other. If we want to be more generic we can base ourselves on the standard (/etc/os-release) and not the results we're looking at today.
|
I'm approving the workflow runs. Overall it's good, but I left comments where I think we can change less. For such a small change, 8 new files and syntax changes, etc. seems a bit much, but let's discuss. |
There was a problem hiding this comment.
I don't think you ran npm run build-dist (as https://github.com/erlef/setup-beam/blob/main/CONTRIBUTING.md proposes) so this'll likely fail CI.
Description
Adds logic to
getRunnerOSVersionto identify the ubuntu version by reading /etc/os-release.I have extracted the function into its own file to avoid complications when importing it for unit testing.
Using fs.readFileSync is not ideal, but I saw that the codebase also makes use of other sync fs functions already. Overall, there are a few refactoring opportunities in the codebase 🙃
To avoid bringing in any new dependency, I made use of node's own test runner which is somewhat new and not perfect, but available on LTS. Node's test runner will run all files ending in
.test.js, which includes the current single test file.As test fixtures, I grabbed the contents of
/etc/os-releasefor three ubuntu and one alpine version and added them to the test directory.Closes #279.