-
Notifications
You must be signed in to change notification settings - Fork 245
feat(compass-schema-validator): add validation action option for 'errorAndLog' with 8.1 COMPASS-8983 #6820
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
Conversation
| export const hasErrorAndLogValidationActionSupport = ( | ||
| serverVersion: string | ||
| ) => { | ||
| return semver.gte(serverVersion, '8.1.0'); |
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.
Sometimes it happens that the version we get from a server is malformed or missing and this will make this code throw breaking the whole tab, we should probably fallback to false in this case
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.
Ah nice catch! I forgot about that, it is an easy pitfall I could see us running into in the future (and has burned us before). We could have a separate shared util for doing this version check, maybe in compass-utils? I can create a ticket if that's something we would want. @gribnoysup
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.
agreed; would be good to abstract away the semver dependency in general as well
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 have this one https://jira.mongodb.org/browse/COMPASS-7129 (we wanted to reopen it, but didn't, I just moved it back to open) and yeah, having something in compass-utils would probably be sufficient as a resolution
| serverVersion: string | ||
| ) => { | ||
| try { | ||
| return semver.gte(serverVersion, '8.1.0-rc.0'); |
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.
seems worthwhile to do this if someone is trying the RC version for whatever reason.
not sure if we'd rather make it stricter and let it just be 8.1.0 as semver is strict about pre-releases otherwise; alternatively we can also semver.coerce(serverVersion)
61fac6a to
bc40db5
Compare
| screen.queryByTestId('validation-action-error-and-log-option') | ||
| ).is.null; | ||
|
|
||
| screen.getByTestId('validation-action-selector').click(); |
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.
Small nit, not sure if there's some special reason to do it that way in your case or not, but if not generally speaking testing library recomments using userEvent for this because it emulates user clicks better (you can see it being done in other tests in this file)
| screen.getByTestId('validation-action-selector').click(); | |
| userEvent.click(screen.getByTestId('validation-action-selector')) |
8.1 supports a new 'errorAndLog' validation action. This adds this to the schema validator UI.
Checklist
Documentation is changed or addedTypes of changes