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

[CHORE] Spelling #3062

Open
wants to merge 219 commits into
base: master
Choose a base branch
from
Open

[CHORE] Spelling #3062

wants to merge 219 commits into from

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Oct 31, 2024

Change description

This PR corrects misspellings identified by the check-spelling action.

This is going to be more of a guide of work as opposed to what's merged.

The misspellings have been reported at https://github.com/jsoref/k8s-config-connector/actions/runs/11616624707#summary-32350007792

The action reports that the changes in this PR would make it relatively happy: https://github.com/jsoref/k8s-config-connector/actions/runs/11616625321#summary-32350008984 (the unrecognized words are fine, I just didn't update the list again, there's a doubled alongside, but that's from another repository so, I'll have to send them a PR separately).

I've tried to be careful about only updating generated items that were changed by things that resulted in CI being happy (my default is to exclude all /generated/ items, but there were some that CI cared about, and some where the masters aren't in this repository, so it was a bit of a balance).

Tests you have done

I created a PR to test CI, check-spelling-sandbox#1 there were a couple (~5) of failures, but they also appeared to be present in master.

  • Run make ready-pr to ensure this PR is ready for review.
  • Perform necessary E2E testing for changed resources.

Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign maqiuyujoyce for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@@ -2,7 +2,7 @@

We'd love to accept your patches and contributions to this project. We use this
GitHub project as our primary source of truth and the main development
repository for Config Connetor. The source code in this project is also
repository for Config Connector. The source code in this project is also
Copy link
Contributor Author

Choose a reason for hiding this comment

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

brand

@@ -114,7 +114,7 @@ repo to quickly set up a local dev environment.
./docker-setup.sh
```

1. Exit your current session, then SSH back in to the VM. Then run the
1. Exit your current session, then SSH back into the VM. Then run the
Copy link
Contributor Author

Choose a reason for hiding this comment

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

into changes tend to be controversial. I'm happy to drop any changes and am certainly not attached to these. From my perspective you're "going into the VM" which is why I think it's correct.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@@ -367,14 +367,14 @@ can run it locally on your dev machine with the steps below.
If you are adding a new resource, you need to follow the steps in [NewResourceFromTerraform.md](README.ChangingTerraform.md)
to make code changes, add test data, and run the tests for your resource.

If you are working on a existing resource, test yaml should exist under
If you are working on an existing resource, test yaml should exist under
Copy link
Contributor Author

Choose a reason for hiding this comment

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

check-spelling ignores 1 and 2 letter words -- changes in this file were by hand because I read the file...

Comment on lines -375 to +376
# Export the environment variables needed in the dynamic tests if you haven't done it.
TEST_FOLDER_ID=123456789 go test -v -tags=integration ./pkg/controller/dynamic/ -test.run TestCreateNoChangeUpdateDelete -run-tests cloudschedulerjob -timeout 900s
```
# Export the environment variables needed in the dynamic tests if you haven't done it.
TEST_FOLDER_ID=123456789 go test -v -tags=integration ./pkg/controller/dynamic/ -test.run TestCreateNoChangeUpdateDelete -run-tests cloudschedulerjob -timeout 900s
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The indentation here didn't match the rest of the file...

@@ -133,7 +133,7 @@ spec:
name: common
template: |
apiVersion: v1
kind: MonfigCap ## should fail
kind: ConfigCap_ ## should fail
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 tend to favor using _s to trigger failures as opposed to having random typos. They're equivalent from a "does not match" and "could be a typo" perspective, but using typos makes it easier for other more problematic typos to be present in a project (see the rest of this PR).

pkg/dcl/kcclite/conversion.go Outdated Show resolved Hide resolved
@@ -251,7 +251,7 @@ func fileToServiceMapping(key string) (*v1alpha1.ServiceMapping, error) {
func parseServiceMapping(b []byte) (*v1alpha1.ServiceMapping, error) {
var sm v1alpha1.ServiceMapping
if err := yaml.UnmarshalStrict(b, &sm); err != nil {
return nil, fmt.Errorf("error unmarshaling byte to service mapping: %w", err)
return nil, fmt.Errorf("error unmarshalling byte to service mapping: %w", err)
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 normally don't harmonize these two spellings, but the proposed spelling is fairly dominant in this repository.

@@ -2161,7 +2161,7 @@ var TestCases = []TestCase{
},
ExpectedResult: []string{"ipAddress"},
},
// (TODO:maqiuyu): Add a test case for resourceID once supported.
// TODO(maqiuyu): Add a test case for resourceID once supported.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is for consistency

@@ -27,7 +27,7 @@
</tr>
<tr>
<td>{{"{{gcp_name_short}}"}} REST Resource Documentation</td>
<td><a href="/dataform/docs/api/reference/rest/v1beta1/projects.locations.repositoriess">/dataform/docs/reference/rest/v1beta1/projects.locations.repositories</a></td>
<td><a href="/dataform/docs/api/reference/rest/v1beta1/projects.locations.repositories">/dataform/docs/reference/rest/v1beta1/projects.locations.repositories</a></td>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notable

@jsoref jsoref changed the title Spelling [CHORE] Spelling Oct 31, 2024
@jsoref jsoref marked this pull request as ready for review November 1, 2024 13:16
@yuwenma
Copy link
Collaborator

yuwenma commented Nov 13, 2024

Thank you @jsoref , very impressive!

I left two comments, please take a look.

@yuwenma
Copy link
Collaborator

yuwenma commented Nov 14, 2024

Hi @jsoref, the repo is developing quickly. Do you think it's possible to split this PR into smaller ones so that the fix can go in quicker? Otherwise, you have to keep on resolving merge conflicts.

@jsoref
Copy link
Contributor Author

jsoref commented Nov 14, 2024

I'm happy to do either. Suggest a split?

Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
@jsoref
Copy link
Contributor Author

jsoref commented Nov 19, 2024

This is a full rebase. There are ~220 commits total. including ~15 link fixes at the top.

I'll try splitting off the link fixes and the first 52 commits and we can see if that's easier...

@jsoref jsoref mentioned this pull request Nov 19, 2024
2 tasks
@jsoref jsoref requested a review from yuwenma November 19, 2024 14:36
@yuwenma
Copy link
Collaborator

yuwenma commented Nov 20, 2024

This is a full rebase. There are ~220 commits total. including ~15 link fixes at the top.

I'll try splitting off the link fixes and the first 52 commits and we can see if that's easier...

Sounds good. #3195 is a good start!

@jsoref
Copy link
Contributor Author

jsoref commented Nov 20, 2024

Ok, let's use ~20 commits at a time, that seems digestible.

In the end, the few items that can't make it due to dependencies can come along in this PR.

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.

2 participants