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

cleanup-book: remove docs/book/src/migration/testdata #1499

Merged
merged 1 commit into from
Aug 14, 2020

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented May 8, 2020

Closes: #1279

The mainly complain of #1279 were the outdated mock testdata in the book which was just used as an example over the final results of the migration from v1 o v2.

In this way, since the V1 is deprecated and no longer for too long then, shows quite safe to remove the old mock data that was just added as an illustration.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 8, 2020
@k8s-ci-robot k8s-ci-robot requested review from Adirio and joelanford May 8, 2020 00:42
@camilamacedo86
Copy link
Member Author

Hi @Adirio @mengqiy,

Could you please help by checking this one? Wdyt? Are you ok with?

k8s.io/client-go v0.18.2
sigs.k8s.io/controller-runtime v0.6.0
sigs.k8s.io/kubebuilder-declarative-pattern v0.0.0-20200507203943-d901af431e3a
sigs.k8s.io/structured-merge-diff v1.0.1-0.20191108220359-b1b620dd3f06 // indirect
Copy link
Member Author

Choose a reason for hiding this comment

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

Indirect deps which were updated with sigs.k8s.io/kubebuilder-declarative-pattern

@Adirio
Copy link
Contributor

Adirio commented May 13, 2020

/hold

See #1279 (comment)

TLDR: Now that the purpose of the testdata folder is clear, IMHO we should not remove it.

@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 May 13, 2020
@camilamacedo86 camilamacedo86 mentioned this pull request May 13, 2020
3 tasks
@camilamacedo86
Copy link
Member Author

camilamacedo86 commented May 13, 2020

HI @mengqiy @droot,

IHMO it is quite safe we remove the V1 example from the books since:

  • V1 code is deprecated, no longer support and removed
  • Users are still able to follow up on the steps. Note that the example do not add too much value at all to the migration guide. PS. If someone has a V1 project and would like to migrate, why is required to keep an example of a V1 project at all? What need this example is addressing?
  • Also, since it uses dep and it is no longer a required at all then, the following error will be faced `

Cannot run program "/Users/camilamacedo/go/bin/dep" (in directory "/Users/camilamacedo/go/src/sigs.k8s.io/kubebuilder/docs/book/src/migration/testdata/gopath/project-v1"): error=2, No such file or directory

WDYT?

c/c @Adirio

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented May 29, 2020

/assign @droot

@camilamacedo86
Copy link
Member Author

hi @pwittrock,

I saw that you have been helping with the reviews. really tks btw. 🥇 So, this one is a case that is ok to be merged but requires more opinions to reach a consensus. if you have time would be great get your input.

@camilamacedo86
Copy link
Member Author

Hi, @gabbifish @pwittrock @DirectXMan12 it is open for a while. WDYT? Are you ok with we removing the mock data of v1 since it is no longer supported a while?

@camilamacedo86
Copy link
Member Author

I am removing the
/hold cancel

Otherwise will be hard to get reviews.

@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 Aug 12, 2020
@pwittrock
Copy link
Contributor

/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 Aug 14, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:
  • OWNERS [camilamacedo86,pwittrock]

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
Copy link
Contributor

@camilamacedo86: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubebuilder-e2e-k8s-1-16-2 3da18c3 link /test pull-kubebuilder-e2e-k8s-1-16-2

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@camilamacedo86
Copy link
Member Author

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

@k8s-ci-robot k8s-ci-robot merged commit ac7b576 into kubernetes-sigs:master Aug 14, 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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs cleanup
4 participants