-
Notifications
You must be signed in to change notification settings - Fork 1.5k
docs: Fix typos and regenerate samples #4766
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
base: master
Are you sure you want to change the base?
docs: Fix typos and regenerate samples #4766
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chethanm99 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 |
Hi @chethanm99. Thanks for your PR. I'm waiting for a kubernetes-sigs 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-sigs/prow repository. |
@@ -20,4 +20,4 @@ jobs: | |||
- name: Run linter | |||
uses: golangci/golangci-lint-action@v6 | |||
with: | |||
version: v1.63.4 | |||
version: v2.0.2 |
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 is not part of the scope of this PR right?
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.
Thanks for the clarification! You were right, the golangci-lint version update is being handled in another issue.
To keep this PR focused, I ran make generate-docs as required, but then manually reverted the golangci-lint version change (from v2.0.2 back to v1.63.4) in the regenerated sample files (lint.yml, Makefile) before committing and squashing.
The PR now contains only the typo fixes and the regenerated files reflecting those fixes, without the unrelated version bump. Everything is squashed into a single commit.
4b8250c
to
6778b35
Compare
@@ -151,7 +150,7 @@ type CronJobStatus struct { | |||
// +optional | |||
Active []corev1.ObjectReference `json:"active,omitempty"` | |||
|
|||
// Information when was the last time the job was successfully scheduled. | |||
// Information when was the last time the job was successfully scheduled ? |
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.
Why ? instead of .
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.
Yes that is a mistake I'll rectify that please let me know if there are any other changes.
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.
Copilot reviewed 28 out of 31 changed files in this pull request and generated 2 comments.
Files not reviewed (3)
- docs/book/src/cronjob-tutorial/testdata/project/Makefile: Language not supported
- docs/book/src/cronjob-tutorial/testdata/project/config/samples/batch_v1_cronjob.yaml: Language not supported
- docs/book/src/multiversion-tutorial/testdata/project/Makefile: Language not supported
We’ll verify that when a CronJob has a single active downstream Job, it's CronJob.Status.The active field contains a reference to this Job. | ||
|
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.
[nitpick] The comment contains a typo: 'it's' should be 'its' and the phrase 'The active field' should be revised to 'Active field' to match convention.
We’ll verify that when a CronJob has a single active downstream Job, it's CronJob.Status.The active field contains a reference to this Job. | |
We’ll verify that when a CronJob has a single active downstream Job, its CronJob.Status.Active field contains a reference to this Job. |
Copilot uses AI. Check for mistakes.
@@ -107,7 +107,7 @@ type CronJobStatus struct { | |||
// +optional | |||
Active []corev1.ObjectReference `json:"active,omitempty"` | |||
|
|||
// Information when was the last time the job was successfully scheduled. | |||
// Information when was the last time the job was successfully scheduled ? |
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.
[nitpick] Remove the extraneous space before the question mark to improve grammatical correctness.
// Information when was the last time the job was successfully scheduled ? | |
// Information when was the last time the job was successfully scheduled. |
Copilot uses AI. Check for mistakes.
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.
Can you please revert the change here?
6778b35
to
72c6500
Compare
Applied the requested changes/nitpicks, @camilamacedo86 |
72c6500
to
edcf163
Compare
@@ -140,7 +140,7 @@ var _ = Describe("CronJob controller", func() { | |||
/* | |||
Now that we've created a CronJob in our test cluster, the next step is to write a test that actually tests our CronJob controller’s behavior. | |||
Let’s test the CronJob controller’s logic responsible for updating CronJob.Status.Active with actively running jobs. | |||
We’ll verify that when a CronJob has a single active downstream Job, its CronJob.Status.Active field contains a reference to this Job. | |||
We’ll verify that when a CronJob has a single active downstream Job, it's CronJob.Status.The active field contains a reference to this Job. |
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 think this one is wrong right?
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 think this one is wrong right?
yes I will also correct this
@@ -151,7 +151,7 @@ var _ = Describe("CronJob controller", func() { | |||
g.Expect(createdCronjob.Status.Active).To(BeEmpty()) | |||
}, duration, interval).Should(Succeed()) | |||
/* | |||
Next, we actually create a stubbed Job that will belong to our CronJob, as well as its downstream template specs. |
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.
Why remove the actually?
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.
yes this is also my mistake i will also resolve this
hackutils.CheckError("add version and marker for storage version", err) | ||
|
||
debugTargetFile := filepath.Join(sp.ctx.Dir, path) // Use 'path' directly |
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.
Why do we need this logic to just update change comments?
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.
Why do we need this logic to just update change comments?
This line was added for debugging process while I was trying to figure out why the make generate-docs script was failing. I will make sure to remove it
@@ -178,7 +178,7 @@ CONTROLLER_TOOLS_VERSION ?= v0.17.2 | |||
ENVTEST_VERSION ?= $(shell go list -m -f "{{ .Version }}" sigs.k8s.io/controller-runtime | awk -F'[v.]' '{printf "release-%d.%d", $$2, $$3}') | |||
#ENVTEST_K8S_VERSION is the version of Kubernetes to use for setting up ENVTEST binaries (i.e. 1.31) | |||
ENVTEST_K8S_VERSION ?= $(shell go list -m -f "{{ .Version }}" k8s.io/api | awk -F'[v.]' '{printf "1.%d", $$3}') | |||
GOLANGCI_LINT_VERSION ?= v1.63.4 | |||
GOLANGCI_LINT_VERSION ?= v2.0.2 |
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 is not part of the scope of this PR?
Could you please rebase with master to ensure that all is properly updated ?
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.
Yes, I will resolve this. Thanks for taking the time to notify this
@@ -128,7 +128,7 @@ func (sp *Sample) UpdateTutorial() { | |||
sp.updateE2E() | |||
} | |||
|
|||
// CodeGen is a noop for this sample, just to make generation of all samples | |||
// CodeGen is a loop for this sample, just to make the generation of all samples |
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.
The comment seems wrong CodeGen will generate the code for the sample maybe is more accurate
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.
Yes you are right i will replace it with the comment you mentioned
edcf163
to
e3f5b97
Compare
@@ -531,11 +531,11 @@ func (r *CronJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct | |||
/* | |||
### Setup | |||
|
|||
Finally, we'll update our setup. In order to allow our reconciler to quickly | |||
look up Jobs by their owner, we'll need an index. We declare an index key that | |||
Finally, we'll update our setup. To allow our reconciler to quickly |
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.
Any reason for not keep In order to
?
@@ -137,7 +137,7 @@ var _ = BeforeSuite(func() { | |||
and it'd be easy to make mistakes. | |||
|
|||
Note that we keep the reconciler running against the manager's cache client, though -- we want our controller to | |||
behave as it would in production, and we use features of the cache (like indices) in our controller which aren't | |||
behave as it would in production, and we use features of the cache (like indices) in our controller that aren'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.
why not keep which?
// jobs at (or anything we missed). | ||
missedRun, nextRun, err := getNextSchedule(&cronJob, r.Now()) | ||
if err != nil { | ||
log.Error(err, "unable to figure out CronJob schedule") | ||
// we don't really care about requeuing until we get an update that |
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.
should we not keep really here? Why not?
// fixes the schedule, so don't return an error | ||
return ctrl.Result{}, nil | ||
} | ||
|
||
/* | ||
We'll prep our eventual request to requeue until the next job, and then figure | ||
out if we actually need to run. |
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.
should we not keep actually here? Why not?
@@ -20,5 +20,6 @@ jobs: | |||
- name: Run linter | |||
uses: golangci/golangci-lint-action@v7 | |||
with: | |||
|
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.
why do we need this space?
Hi @camilamacedo86 I made these typo changes to enhance the readability of the users but some of these changes which you mentioned were not required . I will make sure to make the changes. I am really sorry as this is consuming a lot of your time and I will make sure that I don't make such mistakes in the upcoming contributions. Please let me know if there are any further changes. Thanks once again for your time |
e2d5309
to
5da467a
Compare
// The number of successful finished jobs to retain. | ||
// This is a pointer to distinguish between explicit zero and not specified. | ||
// The number of successfully finished jobs to retain. | ||
// This pointer distinguishes between explicit zero and not specified. |
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 think the original text is accurate, right?
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.
You're right I will replace it with the older text
5da467a
to
b73b99e
Compare
# Note that the option maxDescLen=0 was added in the default scaffold in order to sort out the issue | ||
# Too long: must have at most 262144 bytes. By using kubectl apply to create / update resources an annotation | ||
# Note that the option maxDescLen=0 was added in the default scaffold to sort out the issue | ||
# Too long: must have at most 262144 bytes. By using kubectl to create/update resources an annotation |
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.
Here kubectl apply
is the command so we should keep it.
Maybe we can By using kubectl apply command
@@ -500,7 +500,7 @@ func (r *CronJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct | |||
} | |||
// +kubebuilder:docs-gen:collapse=constructJobForCronJob | |||
|
|||
// actually make the job... |
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 think we should keep actually here
@@ -121,7 +121,7 @@ var _ = Describe("CronJob controller", func() { | |||
/* | |||
Now that we've created a CronJob in our test cluster, the next step is to write a test that actually tests our CronJob controller’s behavior. | |||
Let’s test the CronJob controller’s logic responsible for updating CronJob.Status.Active with actively running jobs. | |||
We’ll verify that when a CronJob has a single active downstream Job, its CronJob.Status.Active field contains a reference to this Job. | |||
We’ll verify that when a CronJob has a single active downstream Job, it's CronJob.Status.The active field contains a reference to this Job. |
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.
here we need to keep as before CronJob.Status.Active
is the spec
docs/book/src/cronjob-tutorial/testdata/project/internal/controller/suite_test.go
Outdated
Show resolved
Hide resolved
b73b99e
to
fa42314
Compare
Hi @camilamacedo86 I've made the necessary changes . I would be happy to make any further changes |
- A way to pause the running of a CronJob, in case something's wrong with it | ||
- Limits on old job history | ||
|
||
Remember, since we never read our own status, we need to have some other way to | ||
keep track of whether a job has run. We can use at least one old job to do | ||
Remember, since we never read our status, we need to have another way to track whether a job has run. We can use at least one old job to do |
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.
Will not it fail in the lint?
The line should still broke as before.
Also, why remove own?
# Note that the option maxDescLen=0 was added in the default scaffold in order to sort out the issue | ||
# Too long: must have at most 262144 bytes. By using kubectl apply to create / update resources an annotation | ||
# Note that the option maxDescLen=0 was added in the default scaffold to sort out the issue | ||
# Too long: must have at most 262144 bytes. By using kubectl to create/update resources an annotation |
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.
Could you please revert the command is kubectl apply that is why we have it as it is.
// fixes the schedule, so don't return an error | ||
return ctrl.Result{}, nil | ||
} | ||
|
||
/* | ||
We'll prep our eventual request to requeue until the next job, and then figure | ||
out if we actually need to run. |
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.
The actually shows make sense here
Can you please revert?
// jobs at (or anything we missed). | ||
missedRun, nextRun, err := getNextSchedule(&cronJob, r.Now()) | ||
if err != nil { | ||
log.Error(err, "unable to figure out CronJob schedule") | ||
// we don't really care about requeuing until we get an update that |
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.
The really shows make sense here as well.
Can you please revert?
fa42314
to
00bd6b5
Compare
# Note that the option maxDescLen=0 was added in the default scaffold in order to sort out the issue | ||
# Too long: must have at most 262144 bytes. By using kubectl apply to create / update resources an annotation | ||
# Note that the option maxDescLen=0 was added in the default scaffold to sort out the issue | ||
# Too long: must have at most 262144 bytes. By using kubectl to create/update resources an annotation |
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.
we need the apply can you please revert
- A way to pause the running of a CronJob, in case something's wrong with it | ||
- Limits on old job history | ||
|
||
Remember, since we never read our own status, we need to have some other way to | ||
keep track of whether a job has run. We can use at least one old job to do | ||
Remember, since we never read our status, we need to have another way to track whether a job has run. We can use at least one old job to do |
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 should be broken in 2 lines, can you please revert
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.
Sure I will revert the changes👍
Corrects various typographical errors in documentation source files and code comments. Runs 'make generate-docs' to ensure generated documentation, tutorials, and sample project files reflect the corrections and are up-to-date. Addresses generation script conflicts caused by specific typo fixes.
00bd6b5
to
4eddf9c
Compare
Hi @camilamacedo86 I've made the requested changes. I would happy to make any further changes. |
This PR fixes multiple typos identified in documentation and comments.
The
make generate-docs
command was run successfully after resolving intermediate generation conflicts. This ensures that all dependent sample projects and documentation components are regenerated using the corrected source material and current project standards.All related changes have been squashed into one commit as required.