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

Fold in knative.dev/client-pkg as a submodule #1953

Merged
merged 18 commits into from
Sep 3, 2024

Conversation

cardil
Copy link
Contributor

@cardil cardil commented Jun 20, 2024

Description

This PR merges code from the knative.dev/client-pkg, that is missing in this repo, onto the knative.dev/client/pkg. TBD a PR to deprecate the whole of the knative.dev/client-pkg.

Changes

  • 🧹 This PR merges code from the knative.dev/client-pkg, that is missing in this repo, onto the knative.dev/client/pkg
  • 🧹 Publicize the code in lib/ dir onto pkg/
  • 🧹 Remove the extra kn sub-package
  • 🧹 Makes pkg/ as a Go submodule and introduces Go's workspace.

Reference

Fixes #1941
Closes #1935
Closes knative/client-pkg#177
Requires knative/hack#386

Release Note

The knative.dev/client-pkg package is now deprecated in favor of the knative.dev/client/pkg package.

Copy link

knative-prow bot commented Jun 20, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 20, 2024
Copy link

@knative-prow knative-prow bot left a comment

Choose a reason for hiding this comment

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

@cardil: 1 warning.

In response to this:

Description

This PR merges code from the knative.dev/client-pkg, that is missing in this repo, onto the knative.dev/client/pkg. TBD a PR to deprecate the whole of the knative.dev/client-pkg.

Changes

  • 🧹 This PR merges code from the knative.dev/client-pkg, that is missing in this repo, onto the knative.dev/client/pkg
  • 🧹 Publicize the code in lib/ dir onto pkg/
  • 🧹 Remove the extra kn sub-package
  • 🧹 Makes pkg/ as a Go submodule and introduces Go's workspace.

Reference

Fixes #1941

Release Note

The [knative.dev/client-pkg](https://pkg.go.dev/knative.dev/client-pkg) is now deprecated in favor of the [knative.dev/client/pkg](https://pkg.go.dev/knative.dev/client/pkg) module.

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.

hack/tools.go Outdated Show resolved Hide resolved
@knative-prow knative-prow bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 20, 2024
@dsimansk
Copy link
Contributor

/test all

Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 50.75758% with 260 lines in your changes missing coverage. Please review.

Project coverage is 75.78%. Comparing base (bb1f899) to head (8ddab3c).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/output/tui/progress.go 0.00% 148 Missing ⚠️
pkg/output/logging/context.go 82.95% 11 Missing and 4 partials ⚠️
pkg/context/wrapper.go 0.00% 14 Missing ⚠️
pkg/output/tui/choose.go 0.00% 14 Missing ⚠️
pkg/output/printer.go 0.00% 12 Missing ⚠️
pkg/output/logging/zap.go 0.00% 9 Missing ⚠️
pkg/config/dir/local.go 76.92% 3 Missing and 3 partials ⚠️
pkg/output/logging/logfile.go 80.00% 3 Missing and 3 partials ⚠️
pkg/output/testing.go 78.57% 4 Missing and 2 partials ⚠️
pkg/output/tui/spinner.go 86.66% 3 Missing and 3 partials ⚠️
... and 9 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1953      +/-   ##
==========================================
- Coverage   76.82%   75.78%   -1.05%     
==========================================
  Files         207      228      +21     
  Lines       12892    13411     +519     
==========================================
+ Hits         9904    10163     +259     
- Misses       2187     2425     +238     
- Partials      801      823      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cardil
Copy link
Contributor Author

cardil commented Jul 2, 2024

/test all

go.mod Outdated Show resolved Hide resolved
@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2024
@cardil
Copy link
Contributor Author

cardil commented Jul 9, 2024

/test all

@cardil
Copy link
Contributor Author

cardil commented Jul 10, 2024

/test build-tests

@cardil
Copy link
Contributor Author

cardil commented Jul 10, 2024

/test unit-tests

@psschwei psschwei removed their request for review July 16, 2024 12:48
@cardil
Copy link
Contributor Author

cardil commented Jul 18, 2024

/test all

@cardil
Copy link
Contributor Author

cardil commented Jul 18, 2024

/test all

@cardil
Copy link
Contributor Author

cardil commented Aug 30, 2024

@dsimansk
Copy link
Contributor

dsimansk commented Sep 2, 2024

Looks like both the github action and the prow tests run just for the main module, not for the pkg:

  1. https://github.com/knative/client/actions/runs/10639800469/job/29498455266?pr=1953
  2. https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_client/1953/unit-tests_client_main/1829644085225328640

Should we go we default implementation of unit tests parsing per go module?
https://github.com/knative/client/blob/main/test/presubmit-tests.sh#L54-L57

@cardil
Copy link
Contributor Author

cardil commented Sep 2, 2024

Should we go we default implementation of unit tests parsing per go module? https://github.com/knative/client/blob/main/test/presubmit-tests.sh#L54-L57

Great catch @dsimansk!

Apart from the above, we need to fix the GitHub Action, as it is the one to publish coverage to Codecov:

https://github.com/knative/actions/blob/8d6af971ff477fdd929367f4c33c865ba0082b13/.github/workflows/reusable-go-test.yaml#L55-L56

.github/workflows/knative-go-build.yaml Outdated Show resolved Hide resolved
.github/workflows/knative-go-test.yaml Outdated Show resolved Hide resolved
@cardil cardil requested a review from dsimansk September 3, 2024 10:04
@dsimansk
Copy link
Contributor

dsimansk commented Sep 3, 2024

/approve
/lgtm

Thanks a lot @cardil!

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2024
Copy link

knative-prow bot commented Sep 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil, dsimansk

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

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2024
@dsimansk
Copy link
Contributor

dsimansk commented Sep 3, 2024

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 3, 2024
@cardil
Copy link
Contributor Author

cardil commented Sep 3, 2024

Maybe I'll do the last one test, before merging - to try using the PKG package in another repo.

@dsimansk
Copy link
Contributor

dsimansk commented Sep 3, 2024

/hold

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 3, 2024
@cardil
Copy link
Contributor Author

cardil commented Sep 3, 2024

The knative-extensions/kn-plugin-event#366 looks to work with this structure without any hiccups.

/hold cancel

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 3, 2024
@knative-prow knative-prow bot merged commit f09e716 into knative:main Sep 3, 2024
23 checks passed
@cardil cardil deleted the chore/fold-in-client-pkg branch September 3, 2024 13:50
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. lgtm 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.

Proposal: Fold back in client-pkg to Go submodule
3 participants