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

[BUG][Connector-V2][Http] fix bug http config no schema option and improve e2e test add case #5939

Merged
merged 9 commits into from
Dec 2, 2023

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
@@ -67,7 +67,7 @@ public class HttpConfig {
public static final Option<ResponseFormat> FORMAT =
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
7 checks passed
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