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

Minor updates to the external Node document #4102

Merged
merged 1 commit into from
Aug 11, 2022

Conversation

jianjuns
Copy link
Contributor

@wenyingd : when reading the doc, I feel these bold font sentences look strange, and also feel not necessary to emphasize them. Let me know what you think.

@jianjuns jianjuns added the kind/documentation Categorizes issue or PR as related to a documentation. label Aug 11, 2022
@jianjuns jianjuns requested a review from wenyingd August 11, 2022 03:49
@wenyingd
Copy link
Contributor

I thought thoses sentences were some key points needed to remind users, that is why I emphasized them. I am OK if you think it is not necessary and remove the mark.

Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

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

LGTM

@jianjuns
Copy link
Contributor Author

I actually got feedback from a reader that he felt he missed some important points after reading the sentences. And these two are actually about internal implementation, which users may not need to care.

@codecov
Copy link

codecov bot commented Aug 11, 2022

Codecov Report

Merging #4102 (a4fe326) into feature/externalnode (274bb85) will increase coverage by 2.99%.
The diff coverage is n/a.

Impacted file tree graph

@@                   Coverage Diff                    @@
##           feature/externalnode    #4102      +/-   ##
========================================================
+ Coverage                 62.66%   65.65%   +2.99%     
========================================================
  Files                       299      302       +3     
  Lines                     45991    46085      +94     
========================================================
+ Hits                      28819    30256    +1437     
+ Misses                    14819    13445    -1374     
- Partials                   2353     2384      +31     
Flag Coverage Δ *Carryforward flag
integration-tests 34.59% <ø> (?)
kind-e2e-tests 49.25% <ø> (ø) Carriedforward from 274bb85
unit-tests 44.09% <ø> (ø) Carriedforward from 274bb85

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/cniserver/ipam/testing/utils.go 92.85% <0.00%> (ø)
...icluster/controllers/multicluster/common/helper.go 58.00% <0.00%> (ø)
pkg/agent/cniserver/testing/utils.go 100.00% <0.00%> (ø)
pkg/agent/util/net.go 45.69% <0.00%> (+0.74%) ⬆️
pkg/agent/openflow/network_policy.go 78.97% <0.00%> (+1.53%) ⬆️
pkg/agent/util/iptables/iptables.go 45.07% <0.00%> (+1.55%) ⬆️
pkg/agent/openflow/framework.go 89.92% <0.00%> (+2.15%) ⬆️
pkg/agent/cniserver/pod_configuration.go 57.66% <0.00%> (+3.06%) ⬆️
pkg/agent/openflow/client.go 69.10% <0.00%> (+3.40%) ⬆️
pkg/agent/types/networkpolicy.go 86.48% <0.00%> (+5.40%) ⬆️
... and 27 more

@wenyingd
Copy link
Contributor

I actually got feedback from a reader that he felt he missed some important points after reading the sentences. And these two are actually about internal implementation, which users may not need to care.

Got it. I think the sentence about "L3 routing is not provided" is an internal implementation, but for the automatic convention from ExternalNode to ExternalEntity is actually what needs to be known by users, so that the users do not need to manually create ExternalEntity.

@jianjuns
Copy link
Contributor Author

jianjuns commented Aug 11, 2022

Sure. But even that one seems to me need not to highlight in the whole doc.

@jianjuns jianjuns merged commit f4589c2 into feature/externalnode Aug 11, 2022
@jianjuns jianjuns deleted the jianjuns-external-node-doc branch August 11, 2022 04:37
wenyingd pushed a commit that referenced this pull request Aug 11, 2022
heanlan pushed a commit to heanlan/antrea that referenced this pull request Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to a documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants