Skip to content
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

fix(KONFLUX-6512): restore allow input CVE for release #97

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JoaoPedroPP
Copy link
Contributor

@JoaoPedroPP JoaoPedroPP commented Jan 28, 2025

Fixes

KONFLUX-6512

Description

Allow to add CVEs while releasing components

Type of change

  • Feature
  • Bugfix
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Screen shots / Gifs for design review

image

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@JoaoPedroPP JoaoPedroPP force-pushed the KONFLUX-6512 branch 2 times, most recently from 4b34b89 to f67cce3 Compare January 28, 2025 15:21
Copy link

codecov bot commented Jan 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.14%. Comparing base (39c0ff7) to head (cfedee1).

Additional details and impacted files
@@           Coverage Diff            @@
##             main      #97    +/-   ##
========================================
  Coverage   80.14%   80.14%            
========================================
  Files         570      570            
  Lines       21536    21536            
  Branches     5076     5332   +256     
========================================
  Hits        17260    17260            
+ Misses       4251     4250     -1     
- Partials       25       26     +1     
Flag Coverage Δ
unittests 80.14% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JoaoPedroPP
Copy link
Contributor Author

I was looking at the history of this file here and in the HAC. I found out that the change from key to id in the CVEFormContent component was because some old field were dropped and had no longer mean and the issue field in form-util.ts has id in the schema instead of key.

The CVEFormContent has the key field in the currently schema

@JoaoPedroPP JoaoPedroPP marked this pull request as ready for review January 28, 2025 18:55
Copy link
Contributor

@CryptoRodeo CryptoRodeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for the reference of the schema.

@@ -40,7 +40,7 @@ const CVEFormContent: React.FC<CVEFormContentProps> = ({ modalToggle }) => {
</TextContent>
</StackItem>
<StackItem>
<InputField data-test="cve-issue-id" label="CVE ID" name="id" required />
<InputField data-test="cve-issue-id" label="CVE ID" name="key" required />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why label is ID and name is key?

Copy link
Contributor Author

@JoaoPedroPP JoaoPedroPP Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice question. Looking at the history of this file in the HAC repository I found this PR#944. There you can find the ticket KFLUXUI-120(KFLUXBUGS-1203) and I understood that is was a design change that replace key for id. However to record the CVE in konflux release the correct schema field name is key. In this case I choose to preserve design and change only the field name to the correct name.

@JoaoPedroPP JoaoPedroPP added this pull request to the merge queue Feb 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 5, 2025
@testcara
Copy link
Contributor

testcara commented Feb 6, 2025

LGTM

@JoaoPedroPP JoaoPedroPP added this pull request to the merge queue Feb 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 6, 2025
@JoaoPedroPP JoaoPedroPP added this pull request to the merge queue Feb 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 6, 2025
@JoaoPedroPP JoaoPedroPP added this pull request to the merge queue Feb 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 7, 2025
@JoaoPedroPP JoaoPedroPP added this pull request to the merge queue Feb 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 7, 2025
@JoaoPedroPP JoaoPedroPP added this pull request to the merge queue Feb 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants