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.

Sign up for GitHub

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

Jump to bottom

doc: update testadata projects used in the tutorial #1632

Merged
merged 1 commit into from
Sep 16, 2020
Merged

doc: update testadata projects used in the tutorial #1632

merged 1 commit into from
Sep 16, 2020

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Aug 10, 2020 •
edited
Loading

Description

  • Upgrade the tesdata used in the tutorial for the latest version since it is very outdated
  • Add script which generates the base of the project at least for be easier we manually perform this process.

Motivation

  • The example is not working and has issues, however, the first step is to update the project for them we are able to address them. The project has many issues raised regards the tutorial testdata. Also, it will show the code in the docs more accurate.

E.g - #1251 #1544

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 10, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86

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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 10, 2020
@camilamacedo86 camilamacedo86 mentioned this pull request Aug 10, 2020
Closed
@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-17-0

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 10, 2020
@camilamacedo86 camilamacedo86 force-pushed the update-docs branch 2 times, most recently from 6c83de3 to af5d380 Compare August 10, 2020 17:16
@camilamacedo86 camilamacedo86 changed the title doc: update cronjob tutorial doc: update testadata projects used in the tutorial Aug 10, 2020
@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-16-2

@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-16-2
/test pull-kubebuilder-e2e-k8s-1-15-3

@fbalicchia
Copy link

Hi @camilamacedo86 ,
in general seems that config files are aligned with new versions kubebuilder 2.3.1

Have tried example test and seems that it fails with namespaces "test-cronjob-namespace" not found.
Please consider that I'm relatively novice and probably I need to familiarize better with env.
In Main.go I've see that to use utilruntime to load external schema interesting but seems that last version of kubebuider doesn't use it and in book seems not mentioned

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Aug 19, 2020 •
edited
Loading

Hi @fbalicchia,

The tutorials here are updated with the latest version on master for the docs be updated for the next release and NOT with kubebuilder 2.3.1. See the script : https://github.com/kubernetes-sigs/kubebuilder/pull/1632/files#diff-95f6fb0104f19012b07b292e718bed49R53.

Regards namespaces "test-cronjob-namespace" it shows that you are not using the make target to deploy it.

However, these projects still with issues and we have many issues tracked over that. But the first step is to keep them updated with the master for them we are able to check each issue and scenario.

camilamacedo86
camilamacedo86 commented Aug 22, 2020
View reviewed changes
docs/book/src/cronjob-tutorial/testdata/project/config/default/manager_webhook_patch.yaml
@@ -9,7 +9,7 @@ spec:
containers:
- name: manager
ports:
- containerPort: 443
- containerPort: 9443
name: webhook-server
protocol: TCP
volumeMounts:
Copy link
Member Author

Choose a reason for hiding this comment

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

It solves https://github.com/kubernetes-sigs/kubebuilder/pull/1645/files

gabbifish
gabbifish reviewed Sep 11, 2020
View reviewed changes
docs/book/src/multiversion-tutorial/testdata/project/main.go
@@ -20,8 +21,8 @@ import (
"flag"
"os"

kbatchv1 "k8s.io/api/batch/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is kbatch1 removed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

should not good catcher

gabbifish
gabbifish reviewed Sep 11, 2020
View reviewed changes
Copy link
Contributor

@gabbifish gabbifish left a comment

Choose a reason for hiding this comment

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

I'm trying to run ./generate_cronjob.sh so I can confirm it works as expected, but it looks like the --plugins flag for kubebuilder init still isn't implemented yet (I set KUBEBUILDER_ENABLE_PLUGINS=1, too!).

I'm using kubebuilder built from top of the master branch so I'm surprised I'm not finding this --plugins flag; maybe there's more I need to configure so I can use this flag and run the testdata generation script?

docs/book/src/cronjob-tutorial/testdata/generate_cronjob.sh
mkdir project
cd project
header_text "creating tutorial.kubebuilder.io base ..."
kubebuilder init --plugins=go/v3-alpha --domain=tutorial.kubebuilder.io --project-version=3-alpha --repo=tutorial.kubebuilder.io/project --license apache2 --owner "The Kubernetes authors"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm using the latest version of kubebuilder (top of master) and it looks like the --plugins flag still doesn't exist here--is that expected / will this come later?

Copy link
Member Author

@camilamacedo86 camilamacedo86 Sep 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @gabbifish,

These scripts are just to allow us re-gen the project inside off this dir and diff with the master to know what requires updates. The plugins only exist on the master branch and by merging this PR we are updating the tutorial with the latest version available which is v3-alpha plugin on the master. It will just be available to production after the next release. We need run make install before to run them.

@camilamacedo86 camilamacedo86 changed the title doc: update testadata projects used in the tutorial WIP: doc: update testadata projects used in the tutorial Sep 13, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2020
@gabbifish gabbifish mentioned this pull request Sep 14, 2020
Closed
@camilamacedo86 camilamacedo86 force-pushed the update-docs branch 2 times, most recently from 257466e to ac3e454 Compare September 14, 2020 12:28
@camilamacedo86 camilamacedo86 changed the title WIP: doc: update testadata projects used in the tutorial doc: update testadata projects used in the tutorial Sep 14, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 14, 2020
@camilamacedo86
Copy link
Member Author

Hi @gabbifish,

Really thank you for your help here. I re-duo everything and re-check this one shows that all is ok.
Note that the multigroup version is passing in the make tests but the cronjob-tutorial not. See: #1680

I think we will sting having issues to address after this update. However, IMO we need first update the examples since they are very outdated for them be able to try to fix them. What IMO we need to ensure here in this PR? That the examples used in the tutorial were not changed at all and that we are not losing any text/explanation.

@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-15-3
/test pull-kubebuilder-e2e-k8s-1-17-0

@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-15-3

/test pull-kubebuilder-e2e-k8s-1-17-0

@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-15-3

@gabbifish
Copy link
Contributor

@camilamacedo86 I reviewed the rest of the PR (except for testing out the --plugins flag, knowing this will be added/expected later). Everything else looks consistent to me :)

@gabbifish
Copy link
Contributor

/lgtm

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

/test pull-kubebuilder-e2e-k8s-1-15-3

@camilamacedo86
Copy link
Member Author

/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 Sep 15, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2020
@camilamacedo86
Copy link
Member Author

/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 15, 2020
gabbifish
gabbifish approved these changes Sep 16, 2020
View reviewed changes
Copy link
Contributor

@gabbifish gabbifish left a comment

Choose a reason for hiding this comment

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

/lgtm

I still cannot test the generate_multiversion scripts given the lack of support for the --plugin flag but it sounds like that is coming shortly? I couldn't use --plugin even when I was using the tip of the master branch. I would love to get a chance to test this feature later, but as you mentioned above, this feature is still coming!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 16, 2020
@k8s-ci-robot k8s-ci-robot merged commit 0273e36 into kubernetes-sigs:master Sep 16, 2020
@gabbifish
Copy link
Contributor

NOO I just found a tiny nit--note the file docs/book/src/multiversion-tutorial/testdata/generate_mutiversion.sh should be called docs/book/src/multiversion-tutorial/testdata/generate_multiversion.sh 😅 I'll make a PR for this tomorrow morning, it's an easy fix

@camilamacedo86
Copy link
Member Author

hi @gabbifish.

really tks for your help with. done: #1683

@camilamacedo86 camilamacedo86 deleted the update-docs branch September 16, 2020 10:07
@camilamacedo86 camilamacedo86 mentioned this pull request Oct 26, 2020
Closed
@camilamacedo86 camilamacedo86 mentioned this pull request Feb 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bradbeam bradbeam bradbeam approved these changes

@gabbifish gabbifish gabbifish approved these changes

@Adirio Adirio Awaiting requested review from Adirio

@DirectXMan12 DirectXMan12 Awaiting requested review from DirectXMan12

Assignees

@gabbifish gabbifish

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@camilamacedo86 @k8s-ci-robot @fbalicchia @gabbifish @bradbeam