Skip to content

src: use Knative Client libraries#167

Merged
zroubalik merged 2 commits intoknative:mainfrom
zroubalik:knclient
Oct 14, 2020
Merged

src: use Knative Client libraries#167
zroubalik merged 2 commits intoknative:mainfrom
zroubalik:knclient

Conversation

@zroubalik
Copy link
Copy Markdown
Contributor

Signed-off-by: Zbynek Roubalik zroubali@redhat.com

Using Knative Client lib for manipulation with Knative (Serving/Eventing) objects

Fixes: #160

@zroubalik zroubalik requested a review from a team October 12, 2020 09:02
@zroubalik zroubalik force-pushed the knclient branch 2 times, most recently from 3c52d7e to d23c3c3 Compare October 12, 2020 10:59
Comment thread knative/remover.go
}

err = client.DeleteService(project, time.Second*30)
err = client.DeleteService(serviceName, time.Second*60)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
Copy link
Copy Markdown
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks ! In general looks very good, except one place where there is still a dependency on the kn commands themselves. Not sure why this is needed but we should get rid of this.

Comment thread knative/client.go Outdated
Comment thread knative/client.go Outdated
Comment thread knative/client.go Outdated
Comment thread knative/describer.go Outdated
Comment thread knative/client.go Outdated
@@ -28,16 +66,5 @@ func NewClient(namespace string, verbose bool) (clientservingv1.KnServingClient,
p.Output = &bytes.Buffer{}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rhuss do you know if there's a way how to capture output, if we remove KnParams{} ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let me check tomorrow, but I don't think that the service API prints anything out on its own.

What actuallly do you want to capture ?

Copy link
Copy Markdown
Contributor Author

@zroubalik zroubalik Oct 13, 2020

Choose a reason for hiding this comment

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

Thanks! If there's nothing to print from the service API, that's easier for us and nothing to capture :)

Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
@zroubalik
Copy link
Copy Markdown
Contributor Author

@rhuss I have updated the PR according to your comments, PTAL :)

Copy link
Copy Markdown
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks !

/approve
/lgtm

Comment thread knative/client.go
)

func NewClient(namespace string, verbose bool) (clientservingv1.KnServingClient, io.Writer, error) {
func NewServingClient(namespace string) (clientservingv1.KnServingClient, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good ! As a future step we might consider to take over those constructors methods to the knative client, too, so that the user doesn't have to specify the config on her own.

Comment thread knative/client.go
"time"

"k8s.io/client-go/tools/clientcmd"
"knative.dev/client/pkg/kn/commands"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@zroubalik zroubalik merged commit 68351bd into knative:main Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use knative client lib

4 participants