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

[ISSUE #4243] Optimize Webhook Manufacturer source Hard-coding #4273

Merged
merged 4 commits into from
Jul 27, 2023

Conversation

Pil0tXia
Copy link
Member

Fixes #4243.

Motivation

image

Some webhook manufacturers' domain names may not follow the www.example.com format with a www subdomain and a .com TLD.

Modifications

Added manufacturerDomain field for WebHookConfig.

Validation of this field has been added to the test classes.

Removed redundant cloudEventSource field because it shouldn't belong to the WebHook Config entity.

Another hardcoding in WebHookProcessor is now replaced by constant.

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

mxsm
mxsm previously approved these changes Jul 23, 2023
Copy link
Member

@mxsm mxsm left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Jul 23, 2023

Codecov Report

Merging #4273 (28158d1) into master (81eacbd) will increase coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head 28158d1 differs from pull request most recent head 08be8aa. Consider uploading reports for the commit 08be8aa to get more accurate results

@@             Coverage Diff              @@
##             master    #4273      +/-   ##
============================================
+ Coverage     16.86%   16.88%   +0.02%     
- Complexity     1427     1428       +1     
============================================
  Files           593      593              
  Lines         26062    26066       +4     
  Branches       2394     2396       +2     
============================================
+ Hits           4396     4402       +6     
- Misses        21223    21224       +1     
+ Partials        443      440       -3     
Files Changed Coverage Δ
...core/protocol/http/processor/WebHookProcessor.java 75.00% <0.00%> (ø)
...e/eventmesh/webhook/receive/WebHookController.java 0.00% <0.00%> (ø)

... and 8 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pandaapo
Copy link
Member

Can you explain why cloudEventSource shouldn't belong to the WebHookConfig entity?

@Pil0tXia
Copy link
Member Author

Pil0tXia commented Jul 24, 2023

@pandaapo Thank you for your attention.

  1. The roles of cloudEventSource and manufacturerDomain are the same.
  2. The properties defined within WebHookConfig should be related to Webhook configurations. The variable names defined in this file will be exposed to users as JSON parameters. The variable naming for manufacturerDomain and manufacturerName is chosen to intuitively indicate the type of values these fields should accept. However, cloudEventSource is an internal concept of the EventMesh and should not be exposed to users or require them to speculate about its meaning, especially in the absence of documentation.
  3. In fact, cloudEventSource currently has no usage and has been deprecated for a long time. The method that should actually call it uses hardcoding (which has been fixed in this PR), and even the test classes incorrectly assign values to it. This indicates that, apart from its definer, no one knows how to use it. This is also one of the reasons for my proposed name modification.

@xwm1992
Copy link
Contributor

xwm1992 commented Jul 25, 2023

@Pil0tXia For this pull request modification, you need to take a look at github webhook usage guide https://eventmesh.apache.org/zh/docs/design-document/webhook, ensuring that the new modification does not affect the normal use of the original function.

@Pil0tXia
Copy link
Member Author

@xwm1992 Yes, I tested. Modifications made in this PR doesn't affect the process logic or the return value of getCloudEventSource.

In fact, the master branch has a NPE when querying webhook configs and I have found the cause. The documents will be updated too.

image

@Pil0tXia
Copy link
Member Author

It's all good. PTAL~ @pandaapo @xwm1992

image

image

Synchronised modification of documents is included in NPE bugfix: apache/eventmesh-site#110

@Pil0tXia
Copy link
Member Author

Pil0tXia commented Jul 27, 2023

@pandaapo Merge conflicts resolved.

@Pil0tXia Pil0tXia requested a review from mxsm July 27, 2023 06:59
@mxsm mxsm merged commit 3652000 into apache:master Jul 27, 2023
7 checks passed
@Pil0tXia Pil0tXia deleted the pil0txia_enhance_4243 branch January 4, 2024 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Optimize Webhook Manufacturer source Hard-coding
5 participants