Skip to content

Web console: Fix required field treatment#11228

Merged
clintropolis merged 1 commit intoapache:masterfrom
implydata:fix-form-default
May 12, 2021
Merged

Web console: Fix required field treatment#11228
clintropolis merged 1 commit intoapache:masterfrom
implydata:fix-form-default

Conversation

@vogievetsky
Copy link
Copy Markdown
Contributor

This PR fixes a bug in the data loader flow when connecting Kinesis.

The issue is that the Endpoint field looks like it is filled in with the default value but it also is required so it requires input.
The issue is fixed as a one line change in ingestion-spec.tsx per the docs the endpoint field is not required.

In addition to fixing this issue:

  • AutoForm has been upgraded so that defaultValue is ignored when required is evaluated to true
  • Tests have been greatly expanded
  • I scanned the code for more instances of this issue and found one in the TIMESTAMP_SPEC_FIELDS
  • Added the ability to Alt+Shift+Click a tile in the ingestion screen to go to its connection step even if the extension is not loaded.

This PR has:

  • been self-reviewed.
  • been tested in a test Druid cluster.

@suneet-s
Copy link
Copy Markdown
Contributor

@vogievetsky Do we know when this bug was introduced? I'm wondering if this is bad enough to justify backporting this to 0.21.1 release.

@vogievetsky
Copy link
Copy Markdown
Contributor Author

@suneet-s the fact that the endpoint field was incorrectly marked as required was there from the start of the data loader but until #10533 it just resulted in an incorrect border color that people would not even notice. #10533 started using the model definitions as validation also and that is when the original bug went from being (from a user perspective) almost unnoticeable to flow breaking. I think it is worth back-porting this.

@suneet-s suneet-s added this to the 0.21.1 milestone May 11, 2021
@suneet-s
Copy link
Copy Markdown
Contributor

@clintropolis fyi I added the milestone 0.21.1 to this bug so we can backport it before making the patch release

@clintropolis clintropolis merged commit d11be88 into apache:master May 12, 2021
clintropolis pushed a commit to clintropolis/druid that referenced this pull request May 12, 2021
clintropolis added a commit that referenced this pull request May 13, 2021
Co-authored-by: Vadim Ogievetsky <vadim@ogievetsky.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants