Skip to content
This repository has been archived by the owner on Mar 9, 2021. It is now read-only.

[kn-source-github] first version of the kn-source-github plugin #46

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

maximilien
Copy link
Contributor

@maximilien maximilien commented Jun 10, 2020

Includes:

  • create, update, delete, list commands
  • first version of unit tests
  • first version of integration tests

Missing:

  • more integration tests. Especially verifying setup of eventing source
  • make IT independent of github user
  • resolve vendor as per new fashion

Fixes #10 and #34

Includes:
* create, update, delete, list commands
* first version of unit tests
* first version of integration tests

Missing:
* more integration tests. Especially verifying setup of eventing source
* make IT independent of github user
* resolve vendor as per new fashion
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 10, 2020
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien

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-robot knative-prow-robot added 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 Jun 10, 2020
@maximilien maximilien changed the title First version of the kn-source-github plugin [kn-source-github] first version of the kn-source-github plugin Jun 10, 2020
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

@@ -0,0 +1,42 @@
/*
Copyright 2019 The Knative Authors
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect copyright year:

Suggested change
Copyright 2019 The Knative Authors
Copyright 2020 The Knative Authors

Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

if err != nil {
fmt.Fprintln(os.Stdout, err)
os.Exit(1)
}
Copy link
Member

Choose a reason for hiding this comment

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

Format Go code:

Suggested change
}
}
}

@maximilien
Copy link
Contributor Author

/hold

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2020
@knative-prow-robot
Copy link

@daisy-ycguo: The /test command needs one or more targets.
The following commands are available to trigger jobs:

  • /test pull-knative-client-contrib-build-tests
  • /test pull-knative-client-contrib-unit-tests
  • /test pull-knative-client-contrib-integration-tests

Use /test all to run all jobs.

In response to this:

/test

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.

@daisy-ycguo
Copy link
Member

/test all

assert.NilError(t, err)

t.Log("kn-source_github create 'source-name' with 'sink-name'")
e2eTest.ghSourceCreate(t, r, "source-name", "sink-name")
Copy link
Member

Choose a reason for hiding this comment

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

I wonder where did you create the sink sink-name. I run test locally, and it reported:

$ kn source_github create source-name --sink sink-name --namespace kne2etests0 --namespace kne2etests0
f.KnSourceParams(): &types.KnSourceParams{KnParams:commands.KnParams{Output:io.Writer(nil), KubeCfgPath:"", ClientConfig:(*clientcmd.DeferredLoadingClientConfig)(0xc0000ec640), NewServingClient:(func(string) (v1.KnServingClient, error))(0x1eb7e20), NewSourcesClient:(func(string) (v1alpha2.KnSourcesClient, error))(0x1eb7ea0), NewEventingClient:(func(string) (v1alpha1.KnEventingClient, error))(0x1eb7f20), NewDynamicClient:(func(string) (dynamic.KnDynamicClient, error))(0x1eb7fa0), LogHTTP:false, fixedCurrentNamespace:""}, SinkFlag:flags.SinkFlags{sink:"sink-name"}}
panic: runtime error: invalid memory address or nil pointer dereference

I think it is because that sink-name is not created.

Copy link
Member

@daisy-ycguo daisy-ycguo Sep 17, 2020

Choose a reason for hiding this comment

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

I wonder if there is a way to set required flags, because kn source_github create source-name --sink sink-name --namespace kne2etests0 shall report error because no GitHub source parameters is provided, logically speaking.

@daisy-ycguo
Copy link
Member

daisy-ycguo commented Sep 17, 2020

@maximilien Hello, dr.max. Currently, the integration test runs nothing, because you don't have an e2e-tests.sh under plugins/source-github/test, just a local-e2e-tests.sh. The integration test framework used by client-contrib is designed to call plugins/source-github/test/e2e-tests.sh for the e2e test of every plugin. So please add a plugins/source-github/test/e2e-tests.sh to your plugin. You may just copy the same of local-e2e-tests.sh. Thanks.

@maximilien
Copy link
Contributor Author

Thanks @daisy-ycguo will do.

@knative-prow-robot
Copy link

@maximilien: PR needs rebase.

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.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2020
Base automatically changed from master to main March 8, 2021 17:37
@knative-prow-robot
Copy link

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

Test name Commit Details Rerun command
pull-knative-client-contrib-integration-tests 897f796 link /test pull-knative-client-contrib-integration-tests
pull-knative-client-contrib-build-tests 897f796 link /test pull-knative-client-contrib-build-tests
pull-knative-client-contrib-unit-tests 897f796 link /test pull-knative-client-contrib-unit-tests

Full PR test history. Your PR dashboard.

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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.

Implement a GitHubSource plugin
5 participants