-
Notifications
You must be signed in to change notification settings - Fork 64
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
feat(release): trigger release form #911
feat(release): trigger release form #911
Conversation
28b7d37
to
d0f1c31
Compare
886e304
to
fb85c3d
Compare
ca9d62b
to
977babd
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #911 +/- ##
==========================================
- Coverage 85.95% 85.88% -0.07%
==========================================
Files 613 627 +14
Lines 15773 16135 +362
Branches 4460 4550 +90
==========================================
+ Hits 13558 13858 +300
- Misses 2075 2136 +61
- Partials 140 141 +1
... and 20 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
}) => { | ||
const [isModalOpen, setIsModalOpen] = React.useState(false); | ||
const [isTimePickerOpen, setIsTimePickerOpen] = React.useState(false); | ||
const dateRef = React.useRef(null); |
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.
Where is this dataRef
being used?
if (dateRef && dateRef.current && dateRef.current.isCalendarOpen) { | ||
dateRef.current.toggleCalendar(false, event.key); | ||
} else if (isTimePickerOpen) { | ||
setIsTimePickerOpen(false); |
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.
dataRef
and isTimePickerOpen
are not being used anywhere
<Table | ||
aria-label="Simple table" | ||
variant="compact" | ||
borders | ||
className="pf-v5-u-m-0 pf-v5-u-p-0" | ||
> |
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.
Why not use the Table component that we use everywhere in list pages?
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.
FieldArray, arrayHelper & index doesn't work properly with our ../shared/Table, using standard patternfly table
|
||
return ( | ||
<Form> | ||
<div ref={formRef}> |
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.
formRef not being used anywhere
e.preventDefault(), handleSubmit(); | ||
modalToggle(); |
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.
e.preventDefault();
handleSubmit();
modalToggle();
|
||
return ( | ||
<FieldArray | ||
name="components" |
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.
this value should be the name
coming from props
// edit | ||
// ? await editReleasePlan(releasePlan, values, workspace, true) | ||
// : await createReleasePlan(values, namespace, workspace, true); |
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.
remove these comments
releasePlan: rp, | ||
snapshot, | ||
data: { | ||
advisory: { |
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.
this should be releaseNotes
cves, | ||
issues, |
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.
talk to errata team to figure out if bugs/cves should be under references
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.
no update yet from errata team. I am using fields as per the latest conversation. I see the object is being created with the correct values in releaseNotes.
Maybe the errata team should remap the values in their API end or file a bug with all the changes they request as there is no document/link provided thus far
src/types/coreBuildService.ts
Outdated
}; | ||
|
||
export type ReleaseSpec = { | ||
snapshot: string; | ||
releasePlan: string; | ||
data?: { | ||
advisory?: { |
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.
change advisory to releaseNotes
releasePlan: rp, | ||
snapshot, | ||
cves, | ||
topic, | ||
labels: labelPairs, | ||
description, | ||
solution, | ||
issues, | ||
synopsis, |
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.
reference field is there in form but not used here
977babd
to
1cdd0be
Compare
9befd3e
to
5ad87eb
Compare
5ad87eb
to
6c80f09
Compare
@abhinandan13jan Everything looks good! Just one small change required. Please add |
{ key: 'inProgress', value: 'In progress' }, | ||
{ key: 'closed', value: 'Closed' }, | ||
{ key: 'resolved', value: 'Resolved' }, |
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.
isBug ? issueTableColumnClass.bugUrl : issueTableColumnClass.bugUrl | ||
} | ||
> | ||
<Truncate content={issue.url} /> |
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.
<StackItem> | ||
<InputField data-test="bug-url" label="URL" name="url" required /> | ||
</StackItem> | ||
<StackItem> |
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.
id: `trigger-releaseplan-${obj.metadata.name}`, | ||
cta: { | ||
href: `/application-pipeline/release/workspaces/${workspace}/release-plan/trigger`, | ||
}, |
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.
Can we add access check review to see if the user has access to create
a release, if not disable the action with tooltip ?
src/pages/TriggerReleasePlanPage.tsx
Outdated
|
||
const TriggerReleasePlanPage: React.FC<React.PropsWithChildren<unknown>> = () => { | ||
const accessReviewResources: AccessReviewResources = [ | ||
{ model: ReleasePlanModel, verb: 'create' }, |
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.
Shouldn't this be ReleaseModel?
{ model: ReleasePlanModel, verb: 'create' }, | |
{ model: ReleaseModel, verb: 'create' }, |
6c80f09
to
8194ec0
Compare
8194ec0
to
46254c2
Compare
@sahil143 @karthikjeeyar updated |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinandan13jan, karthikjeeyar The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
/retest |
1 similar comment
/retest |
@abhinandan13jan: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Fixes
https://issues.redhat.com/browse/HAC-5665
https://issues.redhat.com/browse/HAC-5666
https://issues.redhat.com/browse/HAC-5667
Description
Trigger release base form + add a bug Modal
![ezgif com-resize](https://github.com/openshift/hac-dev/assets/24852534/7794c429-7c2a-4d96-8067-7a6f13300291)Type of change
Screen shots / Gifs for design review
How to test or reproduce?
oc apply -f releasePlan.yaml
Browser conformance: