💝 Extract reusable part of sink flag #1968

cardil · 2024-09-13T14:00:16Z

Description

Changes

💝 Extract reusable part of sink flag

Reference

Fixes #

knative-prow · 2024-09-13T14:00:19Z

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

@cardil: 0 warnings.

In response to this:

Description

Changes

💝 Extract reusable part of sink flag

Reference

Fixes #

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.

codecov · 2024-09-16T16:39:38Z

Codecov Report

Attention: Patch coverage is 80.69498% with 50 lines in your changes missing coverage. Please review.

Project coverage is 75.77%. Comparing base (fd0126d) to head (3a2bcba).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/flags/sink/sink.go	82.40%	15 Missing and 7 partials ⚠️
pkg/k8s/params.go	60.97%	15 Missing and 1 partial ⚠️
pkg/output/logging/context.go	63.63%	3 Missing and 1 partial ⚠️
pkg/output/logging/zap.go	0.00%	4 Missing ⚠️
pkg/dynamic/fake/fake.go	77.77%	1 Missing and 1 partial ⚠️
pkg/util/errors/cause.go	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1968      +/-   ##
==========================================
- Coverage   75.78%   75.77%   -0.01%     
==========================================
  Files         228      234       +6     
  Lines       13411    13538     +127     
==========================================
+ Hits        10163    10259      +96     
- Misses       2425     2451      +26     
- Partials      823      828       +5

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

cardil · 2024-09-16T17:25:51Z

/test all

cardil · 2024-09-16T18:26:21Z

/test all

cardil · 2024-09-17T12:50:16Z

/test all

cardil · 2024-09-18T18:33:43Z

/test all

dsimansk · 2024-09-23T10:18:25Z

@cardil thanks for the PR! I'll go trough the PR later today.

cardil · 2024-09-23T16:45:48Z

pkg/output/logging/log_level.go

@dsimansk I added changes to the logging package, that I use in knative-extensions/kn-plugin-event#368 (just for the continence of working on that kn-event PR)

I can extract those changes to separate PR if you like.

Last two commits, basically:

a386468

3a2bcba

Sure, that's fine. Thanks for highlighting it.

dsimansk · 2024-09-24T15:41:06Z

pkg/commands/subscription/list_test.go

@@ -69,7 +69,7 @@ func TestSubscriptionList(t *testing.T) {
 		assert.NilError(t, err)
 		ol := strings.Split(out, "\n")
 		assert.Check(t, util.ContainsAll(ol[0], "NAME", "CHANNEL", "SUBSCRIBER", "REPLY", "DEAD LETTER SINK", "READY", "REASON"))
-		assert.Check(t, util.ContainsAll(ol[1], "s0", "InMemoryChannel:imc0", "ksvc:ksvc0", "broker:b00", "broker:b01"))
+		assert.Check(t, util.ContainsAll(ol[1], "s0", "InMemoryChannel:imc0", "ksvc0", "broker:b00", "broker:b01"))


@cardil this ksvc prefix was dropped because of changes in SinkToString -> AsText() functions? It might have been midly useful to have a type reference in this list. But not necessary required.

Yes. I wrote the AsText method to be as close as possible to the old one, but I thought that if we default to kservice in the input (ksvc:, and kservice: prefixes are optional), maybe we should also be consistent on the output side?

If you think, we should print those prefixes, even for kservice, then I can change it back.

BTW. Maybe we should deprecate the SinkAsString method (in favor of AsText), as it doesn't care about namespaces. If you target something from a different namespace, it will just print it by name, which is confusing to say the least.

I see, OK let's keep it as optional. Apparently, judging by the test changes we haven't expected a lot of those types prefixes anyway.

dsimansk · 2024-09-24T15:42:21Z

pkg/commands/types.go

-// GetClientConfig gets ClientConfig from KubeCfgPath
-func (params *KnParams) GetClientConfig() (clientcmd.ClientConfig, error) {


Thanks, for cleaning this part. It seems like a very old piece, that doesn't make a lot of sense here.

Yep. Now, the kubernetes related params are in their own struct, which makes far more sense.

BTW. I love the struct composition in Go. Such a great idea, they had...

Ops, in my first comment I thought it's located in different file. I.e. that an isolated command tried to override the config loading. Anyway, it makes sense here, but it's fine to have it extracted as you did. All good here! :)

dsimansk · 2024-09-24T15:45:06Z

@cardil I have one question in subscription list output, but I'm not sure if it's any valid concern towards backwards compability or not. :)

/lgmt
/hold feel free to unhold if the above seems OK.

dsimansk · 2024-09-24T15:45:43Z

/approve

knative-prow · 2024-09-24T15:45:49Z

[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:

~~OWNERS~~ [dsimansk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dsimansk · 2024-09-24T15:46:09Z

/lgtm

dsimansk · 2024-09-25T10:36:34Z

/unhold I'm OK to proceed with the merge.

cardil · 2024-09-25T10:45:32Z

/unhold

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 Sep 13, 2024

knative-prow bot requested review from davidhadas, dsimansk and itsmurugappan September 13, 2024 14:00

knative-prow bot reviewed Sep 13, 2024

View reviewed changes

knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 13, 2024

cardil force-pushed the feature/sink-reuse branch from 171dac3 to aad8c15 Compare September 13, 2024 19:20

knative-prow bot 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 Sep 13, 2024

cardil force-pushed the feature/sink-reuse branch 6 times, most recently from b41d2b2 to 0c0828b Compare September 16, 2024 16:35

cardil force-pushed the feature/sink-reuse branch from 0c0828b to 0a7b3d1 Compare September 16, 2024 17:20

Extract reuseable part of sink flag

f8555e5

cardil force-pushed the feature/sink-reuse branch from 0a7b3d1 to f8555e5 Compare September 16, 2024 18:12

Return nil on empty sink

7410da1

Extract kube params

d6c66ea

knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 18, 2024

Compute with default mappings

d5d174c

cardil marked this pull request as ready for review September 19, 2024 15:21

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 19, 2024

knative-prow bot requested a review from dprotaso September 19, 2024 15:21

Allow to change the default log level

a386468

cardil mentioned this pull request Sep 20, 2024

💝 Migrating to sink flag for --to param knative-extensions/kn-plugin-event#368

Merged

Publicate Zap based logger, and allow custom loggers to be used.

3a2bcba

cardil commented Sep 23, 2024

View reviewed changes

dsimansk reviewed Sep 24, 2024

View reviewed changes

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 24, 2024

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2024

knative-prow bot assigned dsimansk Sep 24, 2024

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

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 25, 2024

knative-prow bot merged commit c9f1284 into knative:main Sep 25, 2024
23 checks passed

cardil deleted the feature/sink-reuse branch September 25, 2024 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

💝 Extract reusable part of sink flag #1968

💝 Extract reusable part of sink flag #1968

cardil commented Sep 13, 2024

knative-prow bot commented Sep 13, 2024

knative-prow bot left a comment

codecov bot commented Sep 16, 2024 •

edited

Loading

cardil commented Sep 16, 2024

cardil commented Sep 16, 2024

cardil commented Sep 17, 2024

cardil commented Sep 18, 2024

dsimansk commented Sep 23, 2024

cardil Sep 23, 2024

cardil Sep 24, 2024

dsimansk Sep 24, 2024

dsimansk Sep 24, 2024 •

edited

Loading

cardil Sep 24, 2024

dsimansk Sep 25, 2024

dsimansk Sep 24, 2024

cardil Sep 24, 2024 •

edited

Loading

dsimansk Sep 25, 2024

dsimansk commented Sep 24, 2024

dsimansk commented Sep 24, 2024

knative-prow bot commented Sep 24, 2024

dsimansk commented Sep 24, 2024

dsimansk commented Sep 25, 2024

cardil commented Sep 25, 2024

		// GetClientConfig gets ClientConfig from KubeCfgPath
		func (params *KnParams) GetClientConfig() (clientcmd.ClientConfig, error) {

💝 Extract reusable part of sink flag #1968

💝 Extract reusable part of sink flag #1968

Conversation

cardil commented Sep 13, 2024

Description

Changes

Reference

knative-prow bot commented Sep 13, 2024

knative-prow bot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

codecov bot commented Sep 16, 2024 • edited Loading

Codecov Report

cardil commented Sep 16, 2024

cardil commented Sep 16, 2024

cardil commented Sep 17, 2024

cardil commented Sep 18, 2024

dsimansk commented Sep 23, 2024

cardil Sep 23, 2024

Choose a reason for hiding this comment

cardil Sep 24, 2024

Choose a reason for hiding this comment

dsimansk Sep 24, 2024

Choose a reason for hiding this comment

dsimansk Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

cardil Sep 24, 2024

Choose a reason for hiding this comment

dsimansk Sep 25, 2024

Choose a reason for hiding this comment

dsimansk Sep 24, 2024

Choose a reason for hiding this comment

cardil Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

dsimansk Sep 25, 2024

Choose a reason for hiding this comment

dsimansk commented Sep 24, 2024

dsimansk commented Sep 24, 2024

knative-prow bot commented Sep 24, 2024

dsimansk commented Sep 24, 2024

dsimansk commented Sep 25, 2024

cardil commented Sep 25, 2024

codecov bot commented Sep 16, 2024 •

edited

Loading

dsimansk Sep 24, 2024 •

edited

Loading

cardil Sep 24, 2024 •

edited

Loading