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

Replace offensive words with alternative ones #2799

Merged
merged 1 commit into from Oct 14, 2021
Merged

Conversation

Timez-zx
Copy link
Contributor

@Timez-zx Timez-zx commented Sep 17, 2021

Replace the terms that require immediate change in word list (master to main, kill to terminate, etc)
Resolve the first expectation of issue #2783

Signed-off-by: Xiao Zhang xzhang2@vmware.com

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

You need to work on the commit message & PR description:

  • "word_change" is not a descriptive commit title. Maybe refer to an earlier PR I had to remove offensive terms from the project: Use more inclusive terminology in project #1702
  • a detailed list of all individual changes is not needed nor helpful. This is what a git diff is for.

The PR should reference the issue, which is #2783 I believe

Finally I believe that you should have a single PR to update the terminology AND add a static checker, as this seems to be what the issue calls for. I don't know if a static checker is super realistic though... at least it will need to provide support for silencing "false positives".

@@ -110,7 +110,7 @@ SLEEP_PID=
function quit {
log_info $CONTAINER_NAME "Stopping OVS before quit"
stop_ovs
# kill background sleep process
# stop background sleep process
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should change a term if it makes the sentence inaccurate
Here we should stick with kill (name of the tool we use) or terminate (name of the signal sent to the process), but a stop signal means something else

maybe the comment can be removed altogether

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

@@ -190,7 +190,7 @@ metadata:
component: antrea-controller
spec:
strategy:
# Ensure the existing Pod is killed before the new one is created.
# Ensure the existing Pod is stoped before the new one is created.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/stoped/stopped

@@ -7,7 +7,7 @@ metadata:
component: antrea-octant
spec:
strategy:
# Ensure the existing Pod is killed before the new one is created.
# Ensure the existing Pod is stoped before the new one is created.
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

docs/egress.md Outdated
@@ -30,7 +30,7 @@ the egress IP when leaving that Node.
You may be interested in using this capability if any of the following apply:

- A consistent IP address is desired when specific Pods connect to services
outside of the cluster, for source tracing in audit logs, or whitelisting
outside of the cluster, for source tracing in audit logs, or aproving
Copy link
Contributor

Choose a reason for hiding this comment

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

it's spelled "approving" but here I think "or for filtering by source IP in external firewall" is a better replacement

@@ -31,7 +31,7 @@ Set-NetIPInterface -ifAlias $INTERFACE_TO_ADD_SERVICE_IP -Forwarding Enabled

if ($StopKubeProxyOnCreation) {
# Restart kube-proxy to ensure that the newly created interface can be used.
# Kill kube-proxy and the process will be automatically restarted by the kube-proxy Pod.
# Stop kube-proxy and the process will be automatically restarted by the kube-proxy Pod.
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as at the beginning

@tnqn
Copy link
Member

tnqn commented Sep 24, 2021

@xiaoxiaobaba You may take a look at https://github.com/antrea-io/antrea/blob/main/CONTRIBUTING.md#getting-reviewers for guidelines about making an appropriate PR. You can "Convert to draft" before you believe the PR is good for review.

@Timez-zx Timez-zx changed the title word_change Replace offensive with alternative words Sep 24, 2021
@Timez-zx Timez-zx changed the title Replace offensive with alternative words Replace offensive words with alternative ones Sep 24, 2021
Copy link
Contributor Author

@Timez-zx Timez-zx left a comment

Choose a reason for hiding this comment

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

The label and description of PR #2799 has been changed and the words used wrongly have also been replaced.

@luolanzone
Copy link
Contributor

luolanzone commented Sep 24, 2021

@xiaoxiaobaba please use your own id to push the commit, otherwise DCO will fail. and you didn't have any title for your commit, I believe you need to review the guide provided by @tnqn to make sure you have the right description.

docs/egress.md Outdated
@@ -30,7 +30,7 @@ the egress IP when leaving that Node.
You may be interested in using this capability if any of the following apply:

- A consistent IP address is desired when specific Pods connect to services
outside of the cluster, for source tracing in audit logs, or whitelisting
outside of the cluster, for source tracing in audit logs, or for filtering
Copy link
Contributor

Choose a reason for hiding this comment

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

you have an extra space before for, please remove it.

@Timez-zx
Copy link
Contributor Author

Because the two commit can't merged into one, so i have to close this PR and build a new one with the same name.

@Timez-zx Timez-zx closed this Sep 24, 2021
@tnqn
Copy link
Member

tnqn commented Sep 24, 2021

Because the two commit can't merged into one, so i have to close this PR and build a new one with the same name.

Two commits can be merged. https://www.atlassian.com/git/tutorials/rewriting-history may be helpful for you.
We'd better to stick to this PR as there have been comments left here.

Besides, the commit message in either PR is still inappropriate.

@Timez-zx
Copy link
Contributor Author

@xiaoxiaobaba You may take a look at https://github.com/antrea-io/antrea/blob/main/CONTRIBUTING.md#getting-reviewers for guidelines about making an appropriate PR. You can "Convert to draft" before you believe the PR is good for review.

I have used “git reset ” to resubmit a new commit under the PR, and the problem have been fixed.

@tnqn
Copy link
Member

tnqn commented Sep 27, 2021

Besides, the commit message in either PR is still inappropriate.

  1. Do not prepend "Title" and "Descripiton" to title and description
  2. Insert an empty line between paragraphs

@Timez-zx
Copy link
Contributor Author

Besides, the commit message in either PR is still inappropriate.

  1. Do not prepend "Title" and "Descripiton" to title and description
  2. Insert an empty line between paragraphs

Done

tnqn
tnqn previously approved these changes Sep 29, 2021
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM, but it seems the email you use for this commit is not associated with your github account, which may cause your account don't get the credit.

@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2021

Codecov Report

Merging #2799 (e2f943f) into main (3a86d2a) will increase coverage by 1.75%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2799      +/-   ##
==========================================
+ Coverage   60.19%   61.95%   +1.75%     
==========================================
  Files         284      283       -1     
  Lines       23475    23735     +260     
==========================================
+ Hits        14131    14705     +574     
+ Misses       7819     7467     -352     
- Partials     1525     1563      +38     
Flag Coverage Δ
kind-e2e-tests 49.57% <ø> (+1.91%) ⬆️
unit-tests 41.60% <ø> (+0.70%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...g/controller/networkpolicy/store/appliedtogroup.go 86.36% <0.00%> (-3.04%) ⬇️
pkg/flowaggregator/flowaggregator.go 67.22% <0.00%> (-2.07%) ⬇️
pkg/agent/metrics/prometheus.go 24.32% <0.00%> (-1.39%) ⬇️
pkg/agent/flowexporter/exporter/exporter.go 80.98% <0.00%> (-1.24%) ⬇️
pkg/agent/nodeportlocal/k8s/npl_controller.go 59.31% <0.00%> (-0.35%) ⬇️
pkg/controller/egress/controller.go 84.12% <0.00%> (ø)
pkg/apis/controlplane/helper.go
pkg/agent/openflow/network_policy.go 83.35% <0.00%> (+0.13%) ⬆️
pkg/agent/agent.go 52.16% <0.00%> (+0.39%) ⬆️
pkg/agent/cniserver/pod_configuration.go 56.00% <0.00%> (+0.44%) ⬆️
... and 18 more

@Timez-zx
Copy link
Contributor Author

LGTM, but it seems the email you use for this commit is not associated with your github account, which may cause your account don't get the credit.

@tnqn The primary e-mail in github has been change to the commit one, maybe it can be merged. And thank you very much for your help.

@tnqn
Copy link
Member

tnqn commented Sep 30, 2021

There is a conflicting file, could you rebase then we can merge?

@Timez-zx
Copy link
Contributor Author

There is a conflicting file, could you rebase then we can merge?

I have rebased the branch, the conflicting file has been solved.

1 similar comment
@Timez-zx
Copy link
Contributor Author

There is a conflicting file, could you rebase then we can merge?

I have rebased the branch, the conflicting file has been solved.

@@ -71,6 +71,7 @@ release. We use `<TAG>` as a placeholder for the release tag (e.g. `v1.4.0`).
particular, the following link should work:
`https://github.com/antrea-io/antrea/releases/download/<TAG>/antrea.yml`.

<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

The conflict is not resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, this conflict should be fixed now.

@Timez-zx Timez-zx force-pushed the x-antrea branch 2 times, most recently from 5b95512 to d5ae24a Compare October 12, 2021 03:28
Replace the terms that require immediate change in wordlist.

Signed-off-by: Xiao Zhang <xzhang2@vmware.com>
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@tnqn
Copy link
Member

tnqn commented Oct 13, 2021

/skip-all

@tnqn tnqn closed this Oct 14, 2021
@tnqn tnqn reopened this Oct 14, 2021
@tnqn
Copy link
Member

tnqn commented Oct 14, 2021

/skip-all

@tnqn
Copy link
Member

tnqn commented Oct 14, 2021

DCO check is missing somehow. I have confirmed the commit is signed off correctly. Merging it.

@tnqn tnqn merged commit 2f21fbd into antrea-io:main Oct 14, 2021
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.

None yet

5 participants