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 credential management section in start pipeline modal. #4955
Add credential management section in start pipeline modal. #4955
Conversation
87bf035
to
f9f7fb0
Compare
/kind feature |
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.
@karthikjeeyar this looks great. Thank you for your patience with these design changes! Approved by UX.
f9f7fb0
to
08c21fd
Compare
/retest |
setAddSecret(false); | ||
}; | ||
|
||
const handleAddSecret = () => { |
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 do we need this functon?
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 callback is used to set state which in turn conditionally render the add button/secret form. These changes are from PR #4868 cc: @vikram-raj
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 function will provide a way to insert the inline form to add the details. Since it on-the-fly creates the Secret (imo a very bad idea) we need this to trigger between those two states.
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.
Yes, this function is for showing and hiding the Create secret form.
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.
What I mean is there is just one line inside the function, isn't it better to use directly, like
(() =>oneline)()
or
oneline()
whichever applies
Just to confirm, if it conforms to the use cases..
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.
So, when I reviewed it was a one-liner so, I thought we need to call it directly...
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.
Based on my last review... recommend just inlining the onClick set value. No need for the extra method.
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.
First pass of comments... will test it now and see what I find.
We'll need a Task that grows with TODOs... there are a few in here that'll need proper attention after this PR is merged.
@@ -0,0 +1,15 @@ | |||
.odc-pipeline-secret-section { | |||
margin-top: calc(-1 * var(--pf-global--spacer--sm)); |
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.
Negative margin is bad. We shouldn't do this.
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 negative margins.
padding: var(--pf-global--spacer--md); | ||
} | ||
&__secret-action { | ||
margin-top: calc(-1 * var(--pf-global--spacer--sm)); |
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 here. There are other layout issues at play if you need negative margin.
<SecretsList namespace={namespace} /> | ||
{addSecret ? ( | ||
<div className="odc-pipeline-secret-section__secret-form"> | ||
<Formik |
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.
Definitely see an issue with form-inside-form concept that Vikram did. I told him the same on his PR. I have since refactored PipelineResources to bubble to the surface of the modal... so we may need to do that here as well... but let me continue reviewing. This may become a tech debt item; doubtful we'll do it part of this PR either way.
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.
Sub task to track : https://issues.redhat.com/browse/ODC-3549
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.
Personally, I don't see an issue with Formik
inside Formik
pattern (not form-inside-form) in these cases. There are several use cases where people have used this pattern and its beneficial when you want to have a separation of concerns between the forms. Parent form doesn't care about all the details of creation of a secret here, it just needs a reference to the created secret. So, the inner secrets form creates all the necessary resources and gives back the reference. We use similar pattern in the Add flows where there is a create secret modal that does the exact same thing.
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.
@rohitkrai03 Technically no, I agree. There are use cases for this. However, this isn't "sub form submit => one property in parent form" - which if it was, I think we'd have a use-case to enjoy this.
We may not need to kill Formik usage itself, but the concept of creating a resource in the middle of a form... especially one in a modal... leads the user to manipulating resources in a context they can "cancel" -- which naturally won't delete the resources.
Personally, I think we can roll it all up to the parent form... but I can see more use-cases, now that I think of it, of having intermediate sub-forms that don't do any "actions" but instead their form submit is just a field update to their parent form.
setAddSecret(false); | ||
}; | ||
|
||
const handleAddSecret = () => { |
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 function will provide a way to insert the inline form to add the details. Since it on-the-fly creates the Secret (imo a very bad idea) we need this to trigger between those two states.
import { SecretAnnotationType } from '../const'; | ||
|
||
type SecretAnnotationParam = { | ||
addLabel?: string; |
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.
Unused.
addLabel?: string; |
]; | ||
|
||
return ( | ||
<Firehose resources={resources}> |
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'd like to see more usage of Watchk8sResource
being used instead of Firehose
it's far superior method to fetching items. But let's put that on the back burner... maybe add it to the list of cleanup stuff - log a task please.
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.
Issue to track : https://issues.redhat.com/browse/ODC-3550
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.
FWIW, I tried using it earlier while I was refactoring a bunch of helm stuff but it just didn't work for me. It went into infinite state updates. Maybe I was doing something wrong there but it led me to fallback to firehose.
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.
@rohitkrai03 Yup, you'll have that problem I suspect if you're trying to get namespaced resources and are creating the object inline.
React.useMemo
is needed to make sure you don't recompute your resources on every render loop... otherwise you'll end up freshly saying "here are different resources" to the hook which naturally tosses everything out and starts again.
Can this be improved so it does a deep compare and we don't need useMemo? Probably. Might be worth mentioning it in #forum-ui.
EDIT: The more I think of it, I wanna say we just remove the need of useMemo - posed the question to the #forum-ui room
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.
Yeah, I figured something of sorts was happening when I tried to use it but didn't wanted to spend much time on it. I definitely think use of useMemo
should not be required and the hook should figure out that the dependency has not been changed automatically. Maybe we can make use of useDeepCompareMemoize()
hook that I created somehow to move this logic into useK8sWatchResource(s)
hooks.
} from '../pipeline-utils'; | ||
import { | ||
constructPipelineData, | ||
mockPipelinesJSON, | ||
mockRunDurationTest, | ||
} from './pipeline-test-data'; | ||
import { SecretAnnotationId } from '../../components/pipelines/const'; |
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.
Sort imports please.
type ServiceAccountKind = { | ||
secrets: { [name: string]: string }[]; | ||
} & K8sResourceCommon; | ||
|
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.
type ServiceAccountKind = { | |
secrets: { [name: string]: string }[]; | |
} & K8sResourceCommon; |
This exists in frontend/public/module/k8s/index.ts
- I know Vikram created this, but we cannot be creating duplicate identifiers just because.
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.
updated types.ts file to use the correct type for secrets
propery in serviceAccountKind.
}); | ||
}; | ||
|
||
type keyValuePair = { |
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.
type keyValuePair = { | |
type KeyValuePair = { |
key: string; | ||
value: string; | ||
}; | ||
export const getTektonSecretAnnotations = (annotation: keyValuePair) => { |
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.
export const getTektonSecretAnnotations = (annotation: keyValuePair) => { | |
export const getSecretAnnotations = (annotation: KeyValuePair): KeyValuePair => { |
@serenamarie125, @siamaksade I have my concerns with Christian about this approach for clarity to the user. I am unsure if We'll likely have to iterate on this, but 🤷 it's a bit awkward to understand what to put in here. |
: setFieldValue('formData', stringData[type]); | ||
}; | ||
|
||
const onDataChanged = (event) => { |
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.
Not an event... it's just the string value.
const onDataChanged = (event) => { | |
const onDataChanged = (value: string) => { |
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.
@karthikjeeyar a missed update?
I'm having some issues with getting the modal to successfully pull from my private repo... I suspect it's a problem between the keyboard and chair. We should have a quick chat @karthikjeeyar tomorrow to see about my issues. Leading concerns (aside from the ones earlier) are:
|
08c21fd
to
096858a
Compare
096858a
to
6af7521
Compare
117f929
to
7e449bd
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.
Not a fan of the way you locked the form... have suggested an alternative that I think works better from a React and Formik point of view.
: setFieldValue('formData', stringData[type]); | ||
}; | ||
|
||
const onDataChanged = (event) => { |
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.
@karthikjeeyar a missed update?
{sortedFilterData.map((secret) => { | ||
return ( | ||
<ResourceLink | ||
className="odc-secrets-list__secretsItem" | ||
key={secret.metadata.uid} | ||
kind={SecretModel.kind} | ||
name={secret.metadata.name} | ||
namespace={secret.metadata.namespace} | ||
title={secret.metadata.name} | ||
/> | ||
); | ||
})} |
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.
Need to read the annotation and at least put the value next to each secret.
<ResourceLink /> (github.com)
@serenamarie125 Not sure where the designs are (I think the back and forth has left the Final Designs doc in a bad state)
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.
Logged a issue to track this https://issues.redhat.com/browse/ODC-3562
secrets?: FirehoseResult<SecretKind[]>; | ||
serviceaccounts?: FirehoseResult<ServiceAccountKind>; |
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.
Not sure these are optional... you haven't given Firehose the option of not providing these (ie, no optional: true
flag on the FirehoseResource)
secrets?: FirehoseResult<SecretKind[]>; | |
serviceaccounts?: FirehoseResult<ServiceAccountKind>; | |
secrets: FirehoseResult<SecretKind[]>; | |
serviceaccounts: FirehoseResult<ServiceAccountKind>; |
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.
Since, firehose is injecting these values, we will get type error on the component. Hence all the firehoseResult components are made as optional accross the codebase.
frontend/packages/dev-console/src/components/pipelines/pipeline-form/StartPipelineForm.tsx
Outdated
Show resolved
Hide resolved
frontend/public/module/k8s/types.ts
Outdated
secrets: SecretKind[]; | ||
secrets: { [name: string]: string }[]; |
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.
Undo. Don't modify public types.
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.
In ServiceAccount type, secrets
is wrongly typed, we need to change or use our own type like vikram added it here in https://github.com/openshift/console/pull/4868/files#r402527823
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 see what you mean, it linked to a K8sResourceKind... yeah that'd be wrong. This is a bad type change though... Log a bug to fix this, this should be better than an open-ended map. There are keys that can be used here.
frontend/packages/dev-console/src/components/pipelines/pipeline-form/PipelineSecretSection.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/pipelines/pipeline-form/StartPipelineForm.tsx
Outdated
Show resolved
Hide resolved
setAddSecret(false); | ||
}; | ||
|
||
const handleAddSecret = () => { |
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.
Based on my last review... recommend just inlining the onClick set value. No need for the extra method.
7e449bd
to
d540d48
Compare
d540d48
to
73aeb1d
Compare
Clicking on the secret, it redirects to the secret detail page but modal remain open. @karthikjeeyar Can we track it as a new issue? |
Yes, we can have a follow up PR for this. Bug to track https://issues.redhat.com/browse/ODC-3566 |
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.
Thanks @karthikjeeyar
/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.
You'll need to seek out Christian for an approval due to the 2 public folder changes.
/assign @christianvogt
/approve |
73aeb1d
to
563551d
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, christianvogt, karthikjeeyar, serenamarie125, vikram-raj 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 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 |
This PR contains an additional commit 7e449bd on top of @vikram-raj's PR-4868
Fixes: https://issues.redhat.com/browse/ODC-3351
Problem:
If pipeline resources are private, then ability to provide secret while starting a pipeline is missing.
Solution:
Added Advanced options in the start pipeline modal and added ability to create new secret and all the linked secrets are listed for user's reference.
Screeshot/gif:
Overall structure of the form:
Pipeline execution succeeded with private git and docker registry:
Unit test coverage:
Browser conformance:
cc: @serenamarie125 @andrewballantyne @vikram-raj