Refactoring code to enable a common package for bicep-deploy #245
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase to enable a common package for bicep-deploy by creating a logging interface and renaming the helpers directory to common.
Key Changes:
- Introduced a
Loggerinterface to abstract logging functionality, enabling dependency injection and better testability - Renamed
helpersdirectory tocommonto better reflect its purpose as a shared package - Created
ActionLoggerclass that implements the Logger interface using GitHub Actions core logging functions
Reviewed Changes
Copilot reviewed 11 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/logging.ts | New file containing the ActionLogger implementation that wraps GitHub Actions logging |
| src/common/logging.ts | Added Logger interface definition; moved logging utilities from helpers to common |
| src/common/whatif.ts | Moved from helpers to common directory |
| src/common/input.ts | Moved from helpers to common directory |
| src/common/file.ts | Updated to accept logger parameter and use Logger interface |
| src/common/azure.ts | Moved from helpers to common directory |
| src/handler.ts | Updated to accept and use Logger interface throughout |
| src/main.ts | Updated imports and instantiates ActionLogger |
| src/config.ts | Updated import path from helpers to common |
| test/handler.test.ts | Updated to instantiate ActionLogger for tests; includes unused import |
| test/file.test.ts | Updated to instantiate ActionLogger for tests |
| test/input.test.ts | Updated import path from helpers to common |
| test/whatif.test.ts | Updated import path from helpers to common |
| test-live/setup.ts | Updated import path from helpers to common |
Comments suppressed due to low confidence (3)
src/common/logging.ts:12
- Spelling error: "ansii" should be "ansi" (ANSI is the correct abbreviation for American National Standards Institute).
src/common/logging.ts:9 - Inconsistent spacing in the Logger interface. Methods
logInfo,logWarning, andlogErrorhave a space before the opening parenthesis, whilelogInfoRawdoes not. Consider removing the space for consistency:logInfo(message: string): any;
test/handler.test.ts:32 - Unused import log.
import { log } from "console";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
interface for input reading.
There was a problem hiding this comment.
New action to upload package bits. For now it can only be invoked manually.
There was a problem hiding this comment.
ignoring so npm pack doesn't package these.
package.json
Outdated
| "package": "esbuild src/index.ts --minify --bundle --outfile=dist/index.js --sourcemap --platform=node --target=es2021", | ||
| "lint": "eslint src test test-live", | ||
| "lint:fix": "eslint src test test-live --fix" | ||
| "lint": "eslint src test test-live packages", |
There was a problem hiding this comment.
I'm wondering if we should avoid having a hierarchy of packages, and instead have these live side by side - to avoid accidental interactions between the two.
E.g. instead of:
/package.json
/packages/bicep-deploy-common/package.json
Should we have something like:
/action/package.json
/common/package.json
Would be a good idea to get @shenglol's opinion on this.
There was a problem hiding this comment.
yes, this is what he suggested using monorepo! i was worried about how it would look when i converted since the PR would be hard to follow.
There was a problem hiding this comment.
We have two options: (1) convert the repo into a monorepo using npm workspaces (optionally with Turborepo), or (2) keep the current structure. I think the second option is fine, but we should decouple the root package.json from bicep-deploy-common/package.json and move the scripts into the latter. We’ll also need to update the root tsconfig.json to exclude the packages folder and add a separate tsconfig.json in bicep-deploy-common.
| { | ||
| "name": "bicep-deploy-common", | ||
| "description": "Common Code for bicep-deploy (GitHub Action/ADO Task)", | ||
| "version": "0.0.1", |
There was a problem hiding this comment.
Do you have a plan for how this version # will get incremented for a release?
There was a problem hiding this comment.
This will have to be manual! It's to allow us to make multiple code changes before revving the package version.
There was a problem hiding this comment.
Got it. I think it would be worth having a discussion and documenting how this should work.
The release process for the action is here: https://github.com/Azure/bicep-deploy/blob/main/scripts/release.sh, however we may want to version the package and action separately, as there are downsides to being too eager with bumping versions for the action. See #141 for example - I was not aware of this when releasing v2!
There was a problem hiding this comment.
I will be adding a separate doc in our internal wiki with information on how to rev the version and upload the package!
| - name: Install Dependencies | ||
| id: install | ||
| run: npm ci | ||
| run: | |
There was a problem hiding this comment.
This negates the purpose of package-lock.json files. If there are conflicts you can delete them and run npm i locally to regenerate them.
There was a problem hiding this comment.
maybe conflicts is the wrong word here - I think the bug I referenced has more info! but it seems like there is a bug around optional dependencies?
| colorize, | ||
| InputReader, | ||
| OutputSetter, | ||
| Logger, |
There was a problem hiding this comment.
Would appreciate any inputs on the naming here!
| try { | ||
| return await action(); | ||
| } catch (ex) { | ||
| if (isRestError(ex)) { |
There was a problem hiding this comment.
Changed this from an instanceof check as I was getting an error where an object that seemingly was of type RestError wasn't being recognized as such. This check is inbuilt into the core-rest-pipeline library and is based on this.
There was a problem hiding this comment.
Currently these files are the same between the common package and live tests. Should we move them somewhere common?
There was a problem hiding this comment.
Does combining all the interface implementations in this file make sense?
This pull request introduces a new shared package, refactors existing code to use it, and modernizes the build, test, and linting workflows. The changes improve code reuse, streamline CI/CD, and update dependencies and tooling for better maintainability.
Introduction of shared package and code refactoring:
@azure/bicep-deploy-commonunderpackages/bicep-deploy-common, including its ownpackage.jsonandREADME.md, to provide common code for both the GitHub Action and ADO Task. The main entry point is nowdist/index.cjs. [1] [2]src/helpers/azure.tstopackages/bicep-deploy-common/src/azure.ts, updated to use new types (DeployConfig,Logger) and improved logging via an interface that can be specific to Action/Task. [1] [2] [3] [4] [5]Build and workflow modernization:
ci.yml,check-dist.yml, newupload-package.yml) to usenpm installinstead ofnpm ci, clearnode_modulesandpackage-lock.jsonbefore install, and build withnpm run build(now usingtsdown). Added steps to build and upload the new common package. [1] [2] [3] [4] [5]Dependency and toolchain updates:
.node-version), switched package entry points to CommonJS (index.cjs), and replaced direct dependencies with a reference to the new common package. [1] [2] [3]Documentation:
packages/bicep-deploy-common/README.md.