chore(KFLUXUI-1072): update component model#712
chore(KFLUXUI-1072): update component model#712marcin-michal wants to merge 2 commits intokonflux-ci:mainfrom
Conversation
Assisted-by: Cursor
marcin-michal
requested review from
rakshett and
testcara
and removed request for
a team
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Component Type Definitions src/types/component.ts |
Added 10 new public types for component configuration modeling (RepositorySettings, ComponentActions, ComponentCreatePipelineConfiguration, ComponentVersion, PipelineDefinition, PipelineSpecFromBundle, PipelineRefGit, ComponentBuildPipeline, ComponentVersionStatus, ComponentStatus). Extended ComponentSource with dockerfileUri and versions fields. Extended ComponentSpecs with actions, repository-settings, skip-offboarding-pr, and default-build-pipeline fields. Replaced ComponentKind inline status with dedicated ComponentStatus type. Added deprecation annotations to mark removal with old component model retirement. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested reviewers
- StanislavJochman
- JoaoPedroPP
- rrosatti
- abhinandan13jan
- Katka92
- janaki29
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'chore(KFLUXUI-1072): update component model' directly describes the main change: updating component model types in src/types/component.ts to reflect API changes. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
marcin-michal
requested review from
JoaoPedroPP,
StanislavJochman,
abhinandan13jan,
janaki29,
milantaky,
rrosatti and
sahil143
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/types/component.ts (2)
16-27: Inconsistent annotation style for future-requirement notes.Lines 16 and 27 use plain
//comments while all other annotations in this file use/** ... */JSDoc style. Plain inline comments don't surface in IDE tooltips on hover, which reduces discoverability for future maintainers.🔧 Proposed consistency fix
export type ComponentSource = { - url?: string; // Will be required when we remove old component model + /** `@todo` Will be required when we remove old component model */ + url?: string; /** `@deprecated` Will be removed when we remove old component model */ git?: { ... }; - dockerfileUri?: string; - versions?: ComponentVersion[]; // Will be required when we remove old component model + dockerfileUri?: string; + /** `@todo` Will be required when we remove old component model */ + versions?: ComponentVersion[]; };The same pattern applies to the plain
//comment on line 144 insideComponentStatus.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/component.ts` around lines 16 - 27, Replace the plain inline `//` annotations with JSDoc-style block comments so they appear in IDE hovers: convert the comment on the `url` property and the comment on the `versions?: ComponentVersion[]` property to `/** ... */` JSDoc form (preserving the existing text "Will be required when we remove old component model"), and do the same for the plain `//` comment inside `ComponentStatus` (around the property at the position noted on line ~144); ensure you only change the comment syntax, not the property names or wording.
56-60:PipelineDefinitiondoesn't enforce mutual exclusivity of its three pipeline ref options.All three fields are independently optional, so TypeScript won't catch cases where zero or multiple refs are provided simultaneously. A discriminated union would make invalid states unrepresentable.
♻️ Proposed stricter type (optional)
-export type PipelineDefinition = { - 'pipelineref-by-git-resolver'?: PipelineRefGit; - 'pipelineref-by-name'?: string; - 'pipelinespec-from-bundle'?: PipelineSpecFromBundle; -}; +export type PipelineDefinition = + | { 'pipelineref-by-git-resolver': PipelineRefGit; 'pipelineref-by-name'?: never; 'pipelinespec-from-bundle'?: never } + | { 'pipelineref-by-name': string; 'pipelineref-by-git-resolver'?: never; 'pipelinespec-from-bundle'?: never } + | { 'pipelinespec-from-bundle': PipelineSpecFromBundle; 'pipelineref-by-git-resolver'?: never; 'pipelineref-by-name'?: never };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/component.ts` around lines 56 - 60, PipelineDefinition currently allows zero or multiple refs because all three properties are optional; replace it with a discriminated union so exactly one ref is provided. Define PipelineDefinition as a union of three object types: one with 'pipelineref-by-git-resolver': PipelineRefGit (and the other two keys absent or typed as never), one with 'pipelineref-by-name': string (others absent/never), and one with 'pipelinespec-from-bundle': PipelineSpecFromBundle (others absent/never); update any callers that construct PipelineDefinition to use the single correct variant and adjust type checks to narrow the union when reading the property.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/types/component.ts`:
- Around line 16-27: Replace the plain inline `//` annotations with JSDoc-style
block comments so they appear in IDE hovers: convert the comment on the `url`
property and the comment on the `versions?: ComponentVersion[]` property to `/**
... */` JSDoc form (preserving the existing text "Will be required when we
remove old component model"), and do the same for the plain `//` comment inside
`ComponentStatus` (around the property at the position noted on line ~144);
ensure you only change the comment syntax, not the property names or wording.
- Around line 56-60: PipelineDefinition currently allows zero or multiple refs
because all three properties are optional; replace it with a discriminated union
so exactly one ref is provided. Define PipelineDefinition as a union of three
object types: one with 'pipelineref-by-git-resolver': PipelineRefGit (and the
other two keys absent or typed as never), one with 'pipelineref-by-name': string
(others absent/never), and one with 'pipelinespec-from-bundle':
PipelineSpecFromBundle (others absent/never); update any callers that construct
PipelineDefinition to use the single correct variant and adjust type checks to
narrow the union when reading the property.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #712 +/- ##
==========================================
- Coverage 87.12% 86.65% -0.47%
==========================================
Files 764 764
Lines 58225 58313 +88
Branches 5658 5658
==========================================
- Hits 50727 50531 -196
- Misses 7430 7605 +175
- Partials 68 177 +109
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 90 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| export type ComponentVersionStatus = { | ||
| 'configuration-merge-url'?: string; | ||
| message?: string; | ||
| name?: string; | ||
| 'onboarding-status'?: string; | ||
| 'onboarding-time'?: string; | ||
| revision?: string; | ||
| 'skip-builds'?: boolean; | ||
| }; | ||
|
|
||
| export type ComponentStatus = { | ||
| /** @deprecated Will be removed when we remove old component model */ | ||
| lastPromotedImage?: string; | ||
| containerImage?: string; | ||
| conditions?: ResourceStatusCondition[]; | ||
| /** @deprecated Will be removed when we remove old component model */ | ||
| devfile?: string; | ||
| /** @deprecated Will be removed when we remove old component model */ | ||
| gitops?: { repositoryURL?: string; branch?: string; context?: string; commitID?: string }; | ||
| /** @deprecated Will be removed when we remove old component model */ | ||
| webhook?: string; | ||
| /** @deprecated Will be removed when we remove old component model */ | ||
| [NudgeStats.NUDGED_BY]?: string[]; | ||
|
|
||
| 'repository-settings'?: RepositorySettings; | ||
| message?: string; | ||
| 'pac-repository'?: string; | ||
| versions?: ComponentVersionStatus[]; // When version is removed from the spec, remove it from the status. | ||
| }; |
There was a problem hiding this comment.
Shouldn't all properties here be required? Also ComponentStatus differs from go file, I'm not sure where these properties came from
There was a problem hiding this comment.
These properties have omitempty option next to them, so I don't think they should be required.
I mostly just moved the existing status properies into a separate type for better readability, adding just the last four on lines 141-144. However, comparing it against the ComponentStatus in the go file, it seems to match, apart from missing one prop LastBuildCommit, is that what you meant? I think it's fine to leave it out as we apparently didn't use it before and it set to be removed in the future.
3 participants
Assisted-by: Cursor
Fixes
KFLUXUI-1072
Description
Updated the component types to reflect changes made in https://github.com/konflux-ci/application-api/blob/main/api/v1alpha1/component_types.go
Type of change
Screen shots / Gifs for design review
How to test or reproduce?
Browser conformance:
Summary by CodeRabbit
New Features
Documentation