feat(update-chart-dependency): implement action (MAPCO-8106) #17
Conversation
|
🎫 Related Jira Issue: MAPCO-8106 |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
| | Name | Description | Required | Default | | ||
| | -------------- | -------------------------------------------------------------------------------------------------------- | -------- | -------- | | ||
| | `service-name` | Name of the dependency to update in Chart.yaml/helmfile.yaml. | true | | | ||
| | `version` | New version to set for the dependency. | true | | |
There was a problem hiding this comment.
why is this mandatory? shouldnt it run on tag push?
There was a problem hiding this comment.
Yes, but the version is mandatory because it needs to be supplied to the action, otherwise we won't know what's the new version (you still need to pass it).
There was a problem hiding this comment.
you can just take the current version of the chart when you run as the default. this action wil run after a version bump anyway.
| [CHART_FILE_NAME, HELMFILE_NAME].forEach((name) => { | ||
| ['yaml', 'yml'].forEach((ext) => { | ||
| const file = path.join(workspace, chartDir, `${name}.${ext}`); | ||
| if (fs.existsSync(file)) { | ||
| files.push(file); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
consider using Iterators for better readability and more functional approach:
const files = Iterator.from([CHART_FILE_NAME, HELMFILE_NAME])
.flatMap(name =>
['yaml', 'yml'].map(ext => path.join(workspace, chartDir, `${name}.${ext}`))
)
.filter(file => fs.existsSync(file))
.toArray();```
Or at least flatmap instead of nested forEach.
```js
const files = [CHART_FILE_NAME, HELMFILE_NAME]
.flatMap(name =>
['yaml', 'yml'].map(ext => path.join(workspace, chartDir, `${name}.${ext}`))
)
.filter(file => fs.existsSync(file));| } catch { | ||
| return { updated: false }; | ||
| } | ||
| if (Array.isArray(chart.dependencies)) { |
There was a problem hiding this comment.
you didnt handle the case that dependencies is not defined
There was a problem hiding this comment.
Added handle and tests.
|
|
||
| vi.mock('@actions/core'); | ||
| vi.mock('@actions/github'); | ||
| vi.mock('fs'); |
There was a problem hiding this comment.
you can use memfs to mock the filesystem and have better tests
Co-authored-by: Ofer <[email protected]>
Co-authored-by: Ofer <[email protected]>
This reverts commit fe62c82.
This reverts commit d63cad8.
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new GitHub Action for automatically updating Helm chart dependencies. The action scans chart directories for Chart.yaml and helmfile.yaml files, updates dependency versions, and creates pull requests with the changes.
Key changes include:
- New update-chart-dependency action with comprehensive input validation using Zod schema
- Support for both Chart.yaml and helmfile.yaml dependency updates with recursive directory scanning
- Automated PR creation with detailed commit messages and descriptions including old/new version tracking
Reviewed Changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| release-please-config.json | Adds release configuration for the new update-chart-dependency action |
| package.json | Adds zod dependency for input validation |
| actions/update-chart-dependency/main.ts | Core implementation with YAML parsing, GitHub API integration, and dependency update logic |
| actions/update-chart-dependency/tests/update-chart-dependency.spec.ts | Comprehensive test suite covering all functionality including edge cases |
| actions/update-chart-dependency/index.ts | Entry point that executes the main action |
| actions/update-chart-dependency/action.yaml | GitHub Action metadata and input definitions |
| actions/update-chart-dependency/README.md | Documentation with usage examples and feature descriptions |
| README.md | Updates main repository documentation to include the new action |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Export the logic in run function to another functions for better readability.
I think it will be better to build the main logic this way:
- Iterate over the target repo
- If a Chart.yaml or helmfile.yaml contains the chart name:
2.1. Create new branch (if the file already has the new version, warn since this shouldn't happen)
2.2. Update file with new version
2.3. Create Pull Request
No need to iterate twice and the update of the version should happen once after you created the branch
| await downloadRepoDir(octokit, owner, repo, branch, '', tempDir); | ||
|
|
||
| // Collect all chart / helmfile files to process | ||
| const chartFilesWithDirs = getChartFilesWithDirs(tempDir); |
There was a problem hiding this comment.
Why getting all the charts? You can filter on the first iteration instead of fetching all the charts and then filter
There was a problem hiding this comment.
I can't do this without reading all of them, this will complicate the logic.
Co-authored-by: Netanel Cohen <[email protected]>
Co-authored-by: Netanel Cohen <[email protected]>
| await downloadRepoDir(octokit, owner, repo, branch, '', tempDir); | ||
|
|
||
| // Collect all chart / helmfile files to process | ||
| const chartFilesWithDirs = getChartFilesWithDirs(tempDir); |
No description provided.