-
Notifications
You must be signed in to change notification settings - Fork 303
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
Spelling #1819
Spelling #1819
Conversation
Welcome @jsoref! |
Hi @jsoref. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most corrections suggested by Google Sheets, all fault mine.
@@ -400,7 +400,7 @@ | |||
**Merged pull requests:** | |||
|
|||
- Cherrypick \#1105 into Release-1.9 [\#1121](https://github.com/kubernetes/ingress-gce/pull/1121) ([freehan](https://github.com/freehan)) | |||
- Cherrpick \#1119 \[Force send Enable field for LogConfig\] into release-1.9 [\#1120](https://github.com/kubernetes/ingress-gce/pull/1120) ([skmatti](https://github.com/skmatti)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some projects don't like changing changelogs. I'm happy to drop any changes (by commit, file, path, etc.)
@@ -41,9 +41,9 @@ func TestASMConfig(t *testing.T) { | |||
}, | |||
{ | |||
desc: "Invalid ConfigMap filed equals to disable", | |||
configMap: map[string]string{"enable-unknow-feild": "INVALID1"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly the spell checker didn't notice unknow
... oh well.
t.Fatalf("Failed to unmarshall backendconfig %s into v1beta1: %v", bcKey, err) | ||
t.Fatalf("Failed to unmarshal backendconfig %s into v1beta1: %v", bcKey, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
projects can disagree on this spelling, but this project almost exclusively uses the shorter spelling.
drainingTransitionPollTimeout = 15 * time.Minute | ||
drainingTansitionPollInterval = 30 * time.Second | ||
drainingTransitionPollTimeout = 15 * time.Minute | ||
drainingTransitionPollInterval = 30 * time.Second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't run a code formatter in general, but i did a quick ski and for files where I guessed the linter would hate me, I let VSCode rewrite the file.
Do note that VSCode does not like the indentation of at least one Markdown file, so I'm not particularly eager to let it micromanage the entire world.
// to use v1 naming naming scheme when master is upgraded to a gke version that use v1.8. | ||
// to use v1 naming scheme when master is upgraded to a gke version that use v1.8. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still getting used to this detector, but I believe I'm right...
negCloud := negtypes.NewFakeNetworkEndpointGroupCloud("test-subnetwork", "test-newtork") | ||
negCloud := negtypes.NewFakeNetworkEndpointGroupCloud("test-subnetwork", "test-network") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible this is intentional, but it didn't seem like it, and, as w/ an earlier change, I encourage projects to use things which are clearly intentional errors as opposed to typos when testing divergence even when the idea is to simulate typos. (Most projects have too many natural typos, and it's really hard to tell them apart from the intentional ones.)
func TestFakeConfigMapVaule(t *testing.T) { | ||
func TestFakeConfigMapValue(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would generally not make a change like this in a go file that wasn't _test
because that'd be a public API, but as this is _test
, I'm hoping it isn't a problem.
// fact in a majority of cases (ubernetes, tests, multiple gke clusters in | ||
// fact in a majority of cases (kubernetes, tests, multiple gke clusters in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brand?
// to handle namespace conflicts in the Ubernetes context. | ||
// to handle namespace conflicts in the Kubernetes context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brand?
// - 5 (hyphn connectors) = 39 | ||
// - 5 (hyphen connectors) = 39 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were two distinct typos of hyphen
w/in the past couple of files. I'm always concerned that something might be a brand as opposed to a typo (e.g. Siemplify
)
Thank you a lot for your changes, Can I ask you to squash commits, and have all changes in a single commit, with good commit message? |
If you're happy with the changes, I'm happy to squash. As the CI hasn't run yet, I have much less faith than I would normally... My default message would be something like:
Possibly with a signed-off-by. But, I'll check to see if there's some obvious style bits I should incorporate... |
This commit didn't have anything fancy in its commit message: bcbe0b9, so, I went w/ the above... |
/ok-to-test |
@panslava: is there something I can do to help this along? periodically rebasing/merging isn't a fun adventure... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jsoref , thank you for your change and sorry it took a while to look into it
In general it feels fine, but can we not change "non-existent" to "nonexistent" ?
* addresses * alpha * annotation * applied * asserting * attachment * attempts * attributes * available * backoff * before * binary * building * bytes-for * cherrypick * cleaned-up * cluster * comparison * condition * config * config-map * container * controller * create * created * custom * daemon * default * deletion * description * destination * discrepancy * do-not * ellipsis * empty * enabled * endpoint * existence * exists * expect * failed * fails * feature * field * for * gcloud * getting * github * given * global * has * have * healthcheck * healthz requests * however * https * https proxy * hyphen * identify * illegal * implements * initialize * instance * instantiated * instead * internal * invalid * kubernetes * labels * lifecycle * lookup * maintain * map-from * matched * metrics * modularity * msg * name * name-space * namespace * naming * network * occurring * omitted * ongoing * overridden * overwrite * path matcher * ports * preexisting * prefix * provided * querying * rate limiting * reclamation * referring * register * regular * replicas * require * resetting * resource * responses * retrieve * retrieving * script * service * shouldn't * specified * stickiness * stuck * switch * syncer * target * targeting * the * throughput * transition * troubleshooting * true * truncated * unable * uneven * unexpected * unknown * unmarshal * unready * unstructured * up to * update * updated * upgrading * utilization balancing * value * verifies * waiting * will * with Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
@panslava: dropped -- do note that it's much easier to drop before squashing. |
/lgtm thank you, sorry again for some delay |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsoref, panslava The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR corrects misspellings identified by the check-spelling action.
The misspellings have been reported at jsoref@c5eb7ab#commitcomment-84112661
The action reports that the changes in this PR would make it happy: jsoref@7314d9a
Note: this PR does not include the action. If you're interested in running a spell check on every PR and push, that can be offered separately.