-
Notifications
You must be signed in to change notification settings - Fork 605
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
Create buildConfig or pipeline based on user input #6874
Create buildConfig or pipeline based on user input #6874
Conversation
79c34a8
to
1a29e2e
Compare
@karthikjeeyar we will also need another story for adding the functionality to edit pipeline using the edit app flow because as I mentioned above previously edit flow used to get git data from buildConfig and if the git data was changed then the new values were added to the buildConfig and it was updated but since now if a pipeline is added there will be no buildConfig and thus I am getting git data from the pipeline for the edit flow but as we do not have the functionality for editing/updating the pipeline the changed data is not propagated back to the pipeline. cc: @serenamarie125 |
@divyanshiGupta When you say git data, is it the url and branch name? If Yes, can we use the Deployemnt/DeploymentConfig/KSVC resource's metadata annotations like app.openshift.io/vcs-ref (branch Name) , app.openshift.io/vcs-uri (URL) to pull the data. WDYT? |
@karthikjeeyar it's not about from where to get the data but if that data is changed it should be propagated to all the resources that use that data in someway. For example the associated pipeline has a git URL param and we can get that data from this param but if git URL changes we need the functionality to update the git URL param of the pipeline thereby updating the pipeline. Currently we don't have the functionality to update the pipeline using edit flows since we were using buildConfig to build the image but now since we are also using pipeline to the build the image we need to have this functionality. It's not just about updating the git params but also we need to decide upon if we want to show the pipeline section and stuff like that and that's where UX input is needed. |
frontend/packages/dev-console/src/components/import/pipeline/pipeline-template-utils.ts
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/import/pipeline/pipeline-template-utils.ts
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/edit-application/EditApplicationPage.tsx
Outdated
Show resolved
Hide resolved
...ackages/dev-console/src/components/edit-application/__tests__/edit-application-utils.spec.ts
Outdated
Show resolved
Hide resolved
@divyanshiGupta I have created a story ODC-5018 to capture the improvements around the edit flows, you have mentioned above. |
82a1d4d
to
c56c357
Compare
Verified locally, works as expected |
/retest |
/kind feature |
c56c357
to
ed18834
Compare
ed18834
to
9b127cd
Compare
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.
lgtm
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.
Few nits and a change I am unsure I understand. Looks like you were removing support for resources but then added support for resources elsewhere.
frontend/packages/dev-console/src/components/import/advanced/AdvancedSection.tsx
Outdated
Show resolved
Hide resolved
case 'DOCKERFILE': | ||
return { ...param, default: dockerfilePath }; |
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.
+1, good catch. This should have been added by the other work. I forgot about the dockerfile flow Pipeline section 🤦
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.
@jerolimov I think we may want to look at back-porting this to piggyback on the work you did for the other params.
frontend/packages/dev-console/src/components/import/pipeline/pipeline-template-utils.ts
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/import/pipeline/pipeline-template-utils.ts
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/import/pipeline/pipeline-template-utils.ts
Outdated
Show resolved
Hide resolved
@@ -92,6 +93,9 @@ export const Status: React.FC<StatusProps> = ({ | |||
case 'Unknown': | |||
return <StatusIconAndText {...statusProps} icon={<UnknownIcon />} />; | |||
|
|||
case 'PipelineNotStarted': | |||
return <StatusIconAndText {...statusProps} icon={<OutlinedHourglassIcon />} />; |
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.
Created a Story to update this icon now they have it figured out. https://issues.redhat.com/browse/ODC-5047
); | ||
} else if (pipeline.template && !dryRun) { | ||
const newPipeline = await createPipelineForImportFlow(formData); | ||
requests.push(createPipelineRunForImportFlow(formData, newPipeline)); |
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.
Also, if this fails (which I don't think is possible today) we'll need to not cancel the submit flow. I've created another story (https://issues.redhat.com/browse/ODC-5048) to chase this down as it may require hacking up the resource in order to test it. No sense in holding up this PR for potentially a non-starter flow.
9b127cd
to
e2db668
Compare
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.
/lgtm
/assign @rohitkrai03 @christianvogt
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: andrewballantyne, bgliwa01, divyanshiGupta, karthikjeeyar, rohitkrai03 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 |
@divyanshiGupta: The following test failed, say
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. |
/retest Please review the full test history for this PR and help us cut down flakes. |
Story: https://issues.redhat.com/browse/ODC-3779
and the app was created using import flows. The latter check is added to make sure that if D/DC was created using CLI/import from yaml then we show the builds section at all times to let the user know that they havent added a buildConfig. Also showing waiting for build alert similar to what is shown in case of buildConfig.Gif:
Unit test coverage: