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

Cherry-pick envtest example #1579

Merged

Conversation

gabbifish
Copy link
Contributor

This PR cherry-picks the new envtest documentation for the cronjob example.

@k8s-ci-robot
Copy link
Contributor

Hi @gabbifish. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 29, 2020
@gabbifish gabbifish changed the title Cherry pick envtest example Cherry-pick envtest example Jun 29, 2020
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 29, 2020
@gabbifish gabbifish changed the base branch from master to book-v2 June 29, 2020 22:52
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 29, 2020
@gabbifish
Copy link
Contributor Author

@camilamacedo86 would love your review on this! :)

@gabbifish
Copy link
Contributor Author

/retest

@k8s-ci-robot
Copy link
Contributor

@gabbifish: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 2, 2020
@gabbifish gabbifish force-pushed the cherry-pick-envtest-example branch from cc5eb53 to b3d11d9 Compare July 2, 2020 22:38
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2020
@camilamacedo86
Copy link
Member

hi @gabbifish,

It is not passing in the tests. See that it is failing the test to check if the testdata was generated. The testdata is updated with the command make generate. More info: CONTRIBUTING.md

However, I do not think that is required to do the cherry-pick the docs will be updated in the next release. But if you wish to check it in the book now then, to make the cherry-pick is required to do the fixes to allow it pass. E.g https://github.com/kubernetes-sigs/kubebuilder/pull/1554/files

@gabbifish
Copy link
Contributor Author

gabbifish commented Jul 3, 2020

@camilamacedo86 Hey there! Yeah, I'm trying to figure out the tricky sigs.k8s.io/kubebuilder-declarative-pattern dependency issue right now. Seems to be a common problem whenever one cherry-picks main branch into book-v2. :( I'll try to fix it. I'm just excited to see my docs live on the kubebuilder book! :)

@gabbifish gabbifish force-pushed the cherry-pick-envtest-example branch from b3d11d9 to 34defe5 Compare July 3, 2020 02:01
@gabbifish
Copy link
Contributor Author

@camilamacedo86 The tests are passing--I think this should be ready for review/merge! :)

@camilamacedo86
Copy link
Member

Hi @gabbifish,

See that your change broke the docs. We need to fix it in the master as soon as possible. Could you please check that?
To verify the docs we use the preview. You can see more info in the CONTRIBUTION.md doc as well.

See that in the master we have: https://book.kubebuilder.io/reference/writing-tests.html
And then, after your changes the page is blank: https://deploy-preview-1579--kubebuilder.netlify.app/reference/writing-tests.html

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

A bug was introduced in the master regards these changes made in the docs. It needs to be fixed to be cherry-picked here as well.

/hold

@camilamacedo86
Copy link
Member

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 8, 2020
@camilamacedo86
Copy link
Member

Hi @gabbifish,

Could you check my above comment? Wdyt about addressing the fix for the docs in master first? Are you able to help with? Do you think that we need to track an issue to fix the docs?

@gabbifish
Copy link
Contributor Author

Hi @camilamacedo86! I went back to try and find the docs rendering errors I might have introduced, but it looks like https://deploy-preview-1579--kubebuilder.netlify.app/cronjob-tutorial/writing-tests.html looks correct now--did the fix from master get cherry-picked into this branch already?

Please let me know if there are still issues!

@camilamacedo86
Copy link
Member

camilamacedo86 commented Jul 31, 2020

Hi @gabbifish,

I think that I found the root cause. See:

In the PR shows that a new sub-menu was created when I understand that we should use the pre-existence one : See:

It should be in the references (writing controller test):
Screenshot 2020-07-31 at 10 32 48

When it is in the tutorial (writing tests):
Screenshot 2020-07-31 at 10 32 40

I think the fix that needs to be done in the master and then cherrry-picked to here 👍

@gabbifish
Copy link
Contributor Author

Cherry-picked the fix from master to this branch! Let me know how things look--I checked that the links to https://deploy-preview-1579--kubebuilder.netlify.app/reference/envtest.html worked from both the sidebar (4.8, Configuring EnvTest) and the link at the top of https://deploy-preview-1579--kubebuilder.netlify.app/cronjob-tutorial/writing-tests.html :D

@estroz
Copy link
Contributor

estroz commented Aug 27, 2020

/retest

@gabbifish
Copy link
Contributor Author

Oop, looks like i need to rebase on this branch again to fix travis CI (I see some dependency issues)

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 31, 2020
…sting-docs

Add envtest testing docs to extend cronjob example
Fix old link to envtest docs that should have been removed
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 31, 2020
@gabbifish
Copy link
Contributor Author

gabbifish commented Aug 31, 2020

cc @camilamacedo86 this should pass Travis CI now, but I ran into some weird dependency problems along the way. Even though this PR does nothing to change the dependencies in the root testdata/ directory, running make check-testdata still required me to apply and add changes to testdata/project-v2-addon/go.mod? Perhaps we shouldn't even be running make check-testdata on the book-v2 branch because the source of truth for testdata/ is whatever is in the master branch...

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 31, 2020
@camilamacedo86
Copy link
Member

camilamacedo86 commented Aug 31, 2020

HI @gabbifish,

Note that the common process is just performed merges here when we do the release and all from the master will be added. However, we also can cherry-pick something from the master but in some scenarios, it might need some adjustments as you did. It is ok for now.

However, I am quite sure that we would be able to improve the process and perform changes. I think ideally we need to raise our suggestions and make a proposal. Maybe this kind of discussion can be started in an issue. WDYT?

@camilamacedo86
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 8, 2020
@camilamacedo86
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 8, 2020
@camilamacedo86
Copy link
Member

It is just a cherry-picks for docs.
PR #1521 which adds the example was checked for my contributors and maintainers.
The second pr #1521 was just to fix the layout of the doc and was checked by me and @pwittrock.

So, it shows fine and required to go alive since many users have been asking for further info over how to test their controllers.
Great contribution @gabbifish 🥇

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 8, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, gabbifish

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3c76661 into kubernetes-sigs:book-v2 Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants