Skip to content
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

Fix showing of server side validation error response in RLP & DNS cre… #70

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

david-martin
Copy link
Contributor

@david-martin david-martin commented Sep 16, 2024

…ate views

closes #58

The Create button on the DNS & RLP pages is disabled until the name, namespace and targetRef are set.
After that, if there's an error when you click create, it will show an alert box below the form (see screenshot for example)

The changes also include the removal of the custom save function for DNSPolicy via the yaml editor as it isn't required (it was originally added as I thought it was needed to convert matchLabels from an object to an array, and back again at the right time for yaml saving, but that's only the case for the form view). Making this change allows the yaml editor to fall back to the built in save function for that component, which handles creation and showing validation errors.

image

TODO:

  • client side validation of resource namespace being non-empty, as an empty namespace causes the k8sCreate fn to POST to a slightly different url (without the namespace in it) which results in a 404, rather than a 422 with a validation error response
  • client side validation of resource name, as it's a standard field to have on all resources, and can easily be reused across all create views (input text type)
  • see if it's possible to highlight validation errors inline on specific fields in a generic way based on the 422 validation error response (or at the very least, have it for name & namespace)

Copy link

openshift-ci bot commented Sep 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: david-martin

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@@ -30,7 +30,7 @@ const NamespaceSelect: React.FC<NamespaceSelectProps> = ({ selectedNamespace, on
onChange(event.currentTarget.value);
};
return (
<FormGroup label={t('Namespace')} fieldId="namespace-select">
<FormGroup label={t('Namespace')} fieldId="namespace-select" isRequired>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the namespace select will likely be removed in favour of NamespaceBar. Adding this now for functionality until that change is made

@david-martin david-martin marked this pull request as ready for review September 17, 2024 15:02
@jasonmadigan
Copy link
Member

👀

Copy link
Member

@jasonmadigan jasonmadigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of small bits

console.error('Error parsing YAML:', error);
}
handleSubmit();
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🫡

<Alert
variant="info"
isInline
title="To set defaults, overrides, and more complex limits, use the YAML view."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

realise this is from an earlier PR, but an i18n string here would be good

@jasonmadigan
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 17, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 17a5d6f into main Sep 17, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Implement and wire up a validation helper to validate policy fields before attempting to save the policy
2 participants