-
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
add default workspace when auto starting pipeline from Add flow #7237
add default workspace when auto starting pipeline from Add flow #7237
Conversation
7d14f6f
to
a4626a3
Compare
/assign |
/assign |
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 uses volumeClaim
by default right ? If that's the case, it works, but this means there might be a few "gotchas":
- 2 pipelines running in parallel might run into problems as they will want to write to the same PVC
- the
git-clone
task needs to set a parameter (that "reset" the workspace before cloning) — that parameter should default to "true" but doesn't as of today I think.
@vdemeester Thank you for the review :)
Hmmm... 2 different pipelines will have 2 different PVC generated here, but for a single pipeline there can be parallel pipelineruns which might use same PVC to read/write. Any suggestions to overcome this issues?
In pipeline 1.2, |
Right, I wasn't clear indeed, I was talking about one pipeilne and two parallel pipelinerun. This could happen "relatively" quickly if this is hooked to a git repository and there is multiple developer on it. Using
Yes 👍 very good then 😝 |
Agreed, PVC isn't a best option in triggerTemplates @vdemeester We have a story to track this ODC-4772, but it dint make it to 4.7 though. |
@@ -9,6 +13,7 @@ import { pipelinesTab } from '../../utils/pipeline-utils'; | |||
import { PipelineRun, getLatestRun } from '../../utils/pipeline-augment'; | |||
import { TektonResourceLabel } from './const'; | |||
import { PipelineRunModel } from '../../models'; | |||
import { PersistentVolumeClaimModel } from '@console/internal/models'; |
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.
nit: move import
@@ -67,6 +72,11 @@ export const createPipelineForImportFlow = async ( | |||
export const createPipelineRunForImportFlow = async (pipeline: Pipeline): Promise<PipelineRun> => { | |||
const pipelineInitialValues: StartPipelineFormValues = { | |||
...convertPipelineToModalData(pipeline), | |||
workspaces: (pipeline.spec.workspaces || []).map((workspace: PipelineWorkspace) => ({ |
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.
is it better to have a null check? pipeline.spec?.workspaces
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.
I have added a fallback || []
above, so it will not break.
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.
sure but if a pipeline doesn't have specs yet, could it through cannot read property workspaces
of undefined
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.
case 'MAJOR_VERSION': | ||
return { ...param, default: 'latest' }; // This needs to be removed when backend sends default value |
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.
Probably should remove this 🤔 It's in the preview
channel, we can install 4.6 which is 1.1.z and that will work fine. They need to update 1.2.z to have this default.
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.
removed it.
@@ -48,6 +51,7 @@ const AddTriggerModal: React.FC<AddTriggerModalProps> = ({ pipeline, close }) => | |||
initialValues={initialValues} | |||
onSubmit={handleSubmit} | |||
validationSchema={addTriggerSchema(t)} | |||
enableReinitialize |
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.
Hmmm, not sure about this... kinda would want to wait for the data before rendering Formik. Reinitializing data could have side-effects mid-usage.
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.
I have removed the enableReInitiaze
prop and waiting for the data to render Formik.
@@ -54,6 +56,7 @@ const StartPipelineModal: React.FC<StartPipelineModalProps & ModalComponentProps | |||
initialValues={initialValues} | |||
onSubmit={handleSubmit} | |||
validationSchema={startPipelineSchema(t)} | |||
enableReinitialize |
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.
Same issue here...
a4626a3
to
0a3a217
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
/retest
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, 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 |
/retest |
1 similar comment
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
1 similar comment
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/test e2e-gcp-console |
Fixes:
https://issues.redhat.com/browse/ODC-5118
Analysis / Root cause:
As a user, when I'm getting my Pipeline Run auto-started off the add flow, I need a PVC volumeClaimTemplate otherwise my Pipeline Run doesn't successfully run.
Solution Description:
PVC
in the Start pipeline modal and Add Trigger modal or fallback to select theEmptyDirectory
optionScreen shots / Gifs for design review:
volumeclaimTemplate
in the pipelinerun).2. In Start Modal, PVC getting auto selected using the pipeline label attached to it.
3. In Add trigger, PVC getting auto selected
Unit test coverage report:
Browser conformance:
cc: @andrewballantyne @jerolimov @vdemeester @openshift/team-devconsole-ux