-
Notifications
You must be signed in to change notification settings - Fork 163
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
Edit for existing project connection instances #3250
Edit for existing project connection instances #3250
Conversation
Skipping CI for Draft Pull Request. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3250 +/- ##
==========================================
+ Coverage 84.85% 84.90% +0.04%
==========================================
Files 1299 1299
Lines 28919 29001 +82
Branches 7770 7807 +37
==========================================
+ Hits 24539 24623 +84
+ Misses 4380 4378 -2
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@@ -111,7 +113,7 @@ const ConnectionTypeForm: React.FC<Props> = ({ | |||
}, | |||
} | |||
} | |||
onDataChange={setConnectionNameDesc} | |||
onDataChange={setConnectionNameDesc ?? (() => undefined)} // onDataChange needs to be truthy to show resource 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.
That's a strange requirement from K8sNameDescriptionField
. Can we update it?
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.
Okay it's been updated. I'm not 100% certain on all the use cases for the component, but i think if you want to old preview behavior, they can still pass in the k8sName.state.immutable = false
const matching = fields?.find((f) => isConnectionTypeDataField(f) && f.envVar === key); | ||
if (!matching) { | ||
unmatched.push({ | ||
type: ConnectionTypeFieldType.ShortText, |
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.
we could be smart. If it contains \n
make it long text, or if it is true|false
make it a boolean. But otherwise I think this works.
response[key] = JSON.parse(decodedString); | ||
} catch { | ||
response[key] = decodedString; |
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 is expected to be an array correct? If so, perhaps ensure it parsed correctly and fallback to [<decodedString>]
?
@@ -74,6 +91,7 @@ const ConnectionTypeFormFields: React.FC<Props> = ({ | |||
<React.Fragment key={i}>{renderDataFields(fieldGroup.fields)}</React.Fragment> | |||
), | |||
)} | |||
{unmatchedValues.length > 0 && renderDataFields(unmatchedValues)} |
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.
@simrandhaliw when editing a connection instance where the env var has no matching field in the connection type, should we section these off with a header and description which informs the user of their situation?
matchingField.properties.variant === 'multi' | ||
) { | ||
try { | ||
response[key] = JSON.parse(decodedString); |
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 this overkill what I'm suggesting to ensure that the parsed value conforms to an array of strings?
response[key] = JSON.parse(decodedString); | |
const parsed = JSON.parse(decodedString); | |
if (Array.isArray(parsed)) { | |
response[key] = parsed.map(v => String(v)); | |
} else { | |
response[key] = [decodedString]; | |
} |
da72b1d
to
e763060
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt 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 |
805ace6
to
5959ccd
Compare
rebasing to run CI again |
/lgtm |
https://issues.redhat.com/browse/RHOAIENG-11556
Description
Hooks up the edit kebab action to the manageConnectionModal. it will pass in the connection of that row, and pass
isEdit
. It will then prefill connection data.The secret stores data in base64, so
atob()
is used to decode, this is done in the modal itself (which is similar to the data connections). This makes it easier to reference a corresponding connection type, to determine how to decode the value.If a connection has env var names that do not match up to a connection type field, they will be appended to the bottom as short text fields.
The multi dropdown is stored as a JSON array, and decoded as such.
The "Save" button isn't enabled until the field values have been updated.
Additionally the k8sNameDesc is now intractable in the preview to mimic the actual form.
How Has This Been Tested?
Tested locally.
Just click the kebab menu on an existing connection row and click "Edit". You can update the values and click Save.
To test non matching values, you modify the env var for the field from the connection type, or edit it directly in openshift
Test Impact
A cypress test added to check the edit functionally
Jest tests to test the modal in edit form
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
@simrandhaliw
After the PR is posted & before it merges:
main