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

Update documents to replace AntreaProxy with Antrea Proxy #6515

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

hongliangl
Copy link
Contributor

No description provided.

@hongliangl hongliangl requested a review from jianjuns July 8, 2024 07:25
@hongliangl hongliangl changed the title Update documents to replace AntreaProxy with Antrea Proxy. Update documents to replace AntreaProxy with Antrea Proxy Jul 8, 2024
@hongliangl hongliangl force-pushed the 20240708-rename-antreaproxy branch from 6c0ecc3 to e8c40a6 Compare July 8, 2024 07:25
jianjuns
jianjuns previously approved these changes Jul 8, 2024
Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

A nit.

docs/design/ovs-pipeline.md Outdated Show resolved Hide resolved
type, `noEncap`, `noSNAT` traffic mode, and `AntreaProxy` feature enabled. Configuration
with `ProxyAll` feature enabled is not verified.
type, `noEncap`, `noSNAT` traffic mode, and Antrea Proxy enabled. Configuration
with `proxyAll` enabled is not verified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Worthy to check all ProxyAll and update ProxyAll to proxyAll if necessary.
I can see there are inconsistent in some docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that every ProxyAll (Without quotting) is at the beginning of sentences. I don't think we need to update them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Only when it refers to the proxyAll field in the YAML, we should we use proxyAll.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only when it refers to the proxyAll field in the YAML, we should we use proxyAll.

I may not get what you mean. Do you mean that we should use proxyAll in the YAML comments, even if the word is at the beginning of sentences?

Copy link
Contributor

Choose a reason for hiding this comment

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

'proxyAllhere should refer to theproxyAllfield inantrea-agent.confright? In this case, we should useproxyAllas Lan suggested, notProxyAll, as the field is named proxyAll`. It does not matter whether it begins a sentence or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, got that. If so, how about this: Option proxyAll?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the current version here "Configuration
with proxyAll enabled is not verified" is fine.

Copy link
Contributor Author

@hongliangl hongliangl Jul 11, 2024

Choose a reason for hiding this comment

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

Thanks, got that. If so, how about this: Option proxyAll?

I meant that how about changing the ProxyAll tells antrea-agent ..... at the beginning of sentences in YAML files to Option proxyAll tells antrea-agent.

I think the current version here "Configuration
with proxyAll enabled is not verified" is fine.

I didn't change this one where proxyAll is not at the beginning of sentences. @jianjuns

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I think we can say "proxyAll tells..."

Copy link
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

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

LGTM

@hongliangl
Copy link
Contributor Author

@jianjuns Could you help do the final confirm to see if we can merge this? Thanks

@jianjuns
Copy link
Contributor

@jianjuns Could you help do the final confirm to see if we can merge this? Thanks

I do not have further comments, but should we wait for your other Antrea Proxy fix PR merged first, and so we can update new added "AntreaProxy" in that PR too?

@hongliangl
Copy link
Contributor Author

@jianjuns Could you help do the final confirm to see if we can merge this? Thanks

I do not have further comments, but should we wait for your other Antrea Proxy fix PR merged first, and so we can update new added "AntreaProxy" in that PR too?

@jianjuns IIRC, I don't have other Antrea Proxy PRs right now. If you meant it is #6308, I have rebased this PR.

@jianjuns
Copy link
Contributor

/skip-all

@jianjuns jianjuns merged commit 63b8117 into antrea-io:main Jul 18, 2024
54 of 58 checks passed
@hongliangl hongliangl deleted the 20240708-rename-antreaproxy branch July 18, 2024 23:05
hongliangl added a commit to hongliangl/antrea that referenced this pull request Jul 19, 2024
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.

3 participants