Skip to content

Conversation

@jackyyyyyssss
Copy link
Contributor

@jackyyyyyssss jackyyyyyssss commented Nov 29, 2023

Purpose of this pull request

fix bug http config no schema option and improve e2e test add case cover

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

@jackyyyyyssss jackyyyyyssss changed the title [Improve][Connector-V2][Http] [BUG][Connector-V2][Http] Nov 29, 2023
@jackyyyyyssss jackyyyyyssss changed the title [BUG][Connector-V2][Http] [BUG][Connector-V2][Http] fix bug http config no schema option and improve e2e test add case Nov 29, 2023
@Hisoka-X Hisoka-X added this to the 2.3.4 milestone Nov 30, 2023
@Hisoka-X Hisoka-X added the bug label Nov 30, 2023
Options.key("format")
.enumType(ResponseFormat.class)
.defaultValue(ResponseFormat.JSON)
.noDefaultValue()
Copy link
Member

Choose a reason for hiding this comment

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

I think you should not change the default value of format. Because it only worked when user configure Http source with schema config. Please refer https://github.com/apache/seatunnel/pull/5939/files#diff-7c704c6a3927123b31b41483102f286339da3832309810496f19410c2548f06fR118-R150

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Removing the default value of JSON is due to validation causing it to never run until there is no schema to take a branch

Copy link
Member

Choose a reason for hiding this comment

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

oh I see. How about add new ResponseFormat TEXT? Then set it as default value. It used when no schema config. Also please update the doc. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done PTAL @Hisoka-X

Copy link
Member

@Hisoka-X Hisoka-X left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jackyyyyyssss

@hailin0 hailin0 merged commit 8a71b9e into apache:dev Dec 2, 2023
alextinng pushed a commit to alextinng/seatunnel that referenced this pull request Dec 19, 2023
@jackyyyyyssss jackyyyyyssss deleted the improve_redis branch July 23, 2024 10:12
chaorongzhi pushed a commit to chaorongzhi/seatunnel that referenced this pull request Aug 21, 2024
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.

4 participants