-
Notifications
You must be signed in to change notification settings - Fork 19
Abstract the @actions/*
modules
#1247
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, apart from the failures to build the tests ;-)
src/__tests__/git.test.ts(36,48): error TS2554: Expected 3-4 arguments, but got 2.
src/__tests__/git.test.ts(69,48): error TS2554: Expected 3-4 arguments, but got 2.
f36f762
to
6a68c44
Compare
Introduce a simple abstraction of the @actions/core module, such that we can provide a different implementation in the future (for Azure Pipelines, for example). Rename 'run' to 'setup' (which is the sibling to 'cleanup'), and add a new 'run' function that takes only an ICore implementation, using @actions/core. Signed-off-by: Matthew John Cheetham <[email protected]>
In a similar way to @actions/core, we abstract the @actions/cache functions. Signed-off-by: Matthew John Cheetham <[email protected]>
Move the shared orchestration logic of the action out of main.ts into a new file src/action.ts. The main.ts file now just calls through to the run() function, with an ICore instance using @actions/core and @actions/cache. Signed-off-by: Matthew John Cheetham <[email protected]>
6a68c44
to
3bcb4c4
Compare
Move any usages of @actions/* packages out of the `src/` subdirectory, and just inline the ActionsCore implementation in the main.ts file. Also there's no need to actually use ActionsCore in the git.test.ts tests, since we're mocking anyway. Just provide an inline no-op implementation. Signed-off-by: Matthew John Cheetham <[email protected]>
Didn't we move away from Azure Pipelines? Is this a microsoft/git thing? Is this just the CI system that's easiest to produce a proof of concept use of the abstraction for? |
Yes, we did. Back when we started this move, the official communication was that Azure DevOps users are highly encouraged to move to GitHub. For... reasons... this seems to have changed.
Yes. For... reasons...
It's more like this is the reason for the abstraction. |
Most of the logic of this GitHub Action is portable to other CI systems, apart from those functions in
@actions/core
and@actions/cache
.Introduce an abstraction
ICore
and a corresponding implementation for GitHub Actions that is just a very thin layer over the@actions/* module
functions.The only logic change here is the introduce of this block:
Other CI systems may not support caching, so we add a check for this and disable the caching behaviour if not available. For the GitHub Actions implementation of
ICore
, we could just returntrue
, but there is also anisFeatureAvailable()
function in@actions/cache
that we should probably have always been checking.In a future PR I plan to add a very simple 'shim' or wrapper to expose an Azure Pipelines task.