Move AWS SQS source to contrib #253

srvaroa · 2019-03-16T15:53:14Z

Along with some documentation cleanup and a couple of simplifications of code. Tested on v0.4.0.

srvaroa · 2019-03-16T15:56:39Z

Superseeds #177 (cc @n3wscott @matzew who were reviewing that one)

contrib/awssqs/pkg/apis/sources/v1alpha1/aws_sqs_types_test.go

matzew · 2019-03-18T08:27:58Z

contrib/awssqs/samples/dumper-resources.yaml

@@ -23,8 +36,7 @@ spec:
 apiVersion: serving.knative.dev/v1alpha1
 kind: Service
 metadata:
-  name: message-dumper
-  namespace: default
+  name: awssqs-message-dumper


let's use the new display events thing from @n3wscott

#249

I was watching that one, my plan was to update once it was merged.

evankanderson · 2019-03-29T18:46:50Z

Is this ready for re-review? It looks like there were some conflicts that slipped in recently, but I can take a look soon if this is ready.

srvaroa · 2019-03-29T23:28:47Z

@evankanderson yes it is ready, I have just resolved the conflicts. Some conflicts were in the documentation so I will test it to be sure it's ok, but otherwise the PR should be ready as is.

Thanks in advance!

srvaroa · 2019-03-30T08:59:35Z

/retest

contrib/awssqs/samples/README.md

daisy-ycguo · 2019-04-09T06:13:46Z

contrib/awssqs/samples/README.md

+Deploy the `AwsSqsSource` controller as part of eventing-source's controller.
+
+   ```shell
+   ko -n default apply -f config/


This is wrong. It should be ko -n default apply -f contrib/awssqs/

It should be correct but it's relative to contrib/awssqs. The Deploy section above indicates that commands are run from that dir.

I understand however that you prefer to have the commands relative to the root of the repo (that is the case for instructions in other sources in contrib/). I'll change it.

Done 26f79e2

daisy-ycguo · 2019-04-09T06:17:23Z

contrib/awssqs/samples/README.md

+```
+
+You can use [kail](https://github.com/boz/kail/) to tail the logs of the
+subscriber.


Other sources use kubectl log to monitor the logs of event-display other than asking people to install a new tool kail. I will suggest to use kubectl log here for consistency. Of course, you can also add guidances about kail together with kubectl log.

Kail is used in all the eventing and eventing-sources documentation, and the previous version of awssqs (this doc is mostly being moved). Examples are gcp pubsub L153 or Camel at L73, L115.

I agree though that the less tools we expect the better, I can do a round and update all these docs, so we remain consistent?

+1 for removing kail, we've been removing it as we go forward, but things linger. Perhaps we can just create an issue to track removing it from all the examples and do it in a followup.

n3wscott · 2019-04-09T15:40:57Z

We should get this in asap to stop diverging. Sorry about the time delay.

Can you confirm that the moved source is in-sync with the non-contrib version? If so we can merge and then do followup PRs to clean up.

srvaroa · 2019-04-09T17:26:55Z

@n3wscott that'd be great :) This is pretty much the same code as in non-contrib (save for what was necessary to change to make it fully independent inside contrib). I am happy to follow up with improvements / fixes in other PRs.

srvaroa · 2019-04-09T17:42:08Z

@n3wscott I suspect the failure in integration tests is not really due to the PR (the last commits only change markdown files.) I'll try retriggering a bit later, but if you think a retry would work and can /retest for me that'd be awesome.

grantr · 2019-04-09T18:20:25Z

/retest

srvaroa · 2019-04-10T10:14:17Z

@n3wscott looks like this is mergeable now, let me know if you'd like to see the changes about kail done before (see thread).

srvaroa · 2019-04-20T16:54:22Z

Force-push fixing merge conflicts.

srvaroa · 2019-04-24T17:59:49Z

@n3wscott just a kind ping in case this slipped through your radar :)

Signed-off-by: Galo Navarro <[email protected]>

srvaroa · 2019-05-06T19:13:48Z

Thanks @evankanderson

srvaroa · 2019-05-06T19:30:25Z

/retest

evankanderson

You might want to do a quick grep for VerbAll -- I missed one in my previous scan of the code.

evankanderson · 2019-05-08T16:47:14Z

contrib/awssqs/config/201-clusterrole.yaml

@@ -22,24 +22,24 @@ rules:
  resources:
  - deployments
  verbs:
-  - VerbAll
+  - *
 - apiGroups:
  - ""
  resources:
  - events
  verbs:
  - VerbAll


You have another "VerbAll" here

I did miss this one, thanks! I could not find any others.

Signed-off-by: Galo Navarro <[email protected]>

knative-metrics-robot · 2019-05-08T17:28:15Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-sources-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
contrib/awssqs/pkg/adapter/adapter.go	Do not exist	61.8%
contrib/awssqs/pkg/apis/sources/v1alpha1/aws_sqs_types.go	Do not exist	100.0%
contrib/awssqs/pkg/apis/sources/v1alpha1/register.go	Do not exist	100.0%

evankanderson

/lgtm

nachocano · 2019-05-10T02:23:49Z

Just merged master and this seems to be broken. Can you make one more pass @srvaroa?

ko apply -f ./contrib/awssqs/config/ complains with "error processing import paths in "contrib/awssqs/config/201-clusterrole.yaml": yaml: line 25: did not find expected alphabetic or numeric character". It seems you cannot use * for specifying the verbs, we will have to list them all.

Also, the controller never starts as we are not importing the eventing scheme anymore. We need a addtoscheme_eventing_v1alpha1.go inside apis package. You can copy it from gcppubsub.

Thanks!

srvaroa · 2019-05-10T06:57:57Z

Thanks for the heads up @nachocano!

Pushing these fixes in #406 I can't test them right now on a cluster but will do over the weekend.

nachocano · 2019-05-10T16:14:18Z

I can't test them right now on a cluster but will do over the weekend

Great! Thanks @srvaroa!

knative-prow-robot requested review from evankanderson and grantr March 16, 2019 15:53

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 16, 2019

matzew reviewed Mar 18, 2019

View reviewed changes

contrib/awssqs/pkg/apis/sources/v1alpha1/aws_sqs_types_test.go Show resolved Hide resolved

matzew reviewed Mar 18, 2019

View reviewed changes

srvaroa force-pushed the master branch 3 times, most recently from 2139489 to 1ae86cf Compare March 23, 2019 09:29

srvaroa force-pushed the master branch from 1ae86cf to 8428521 Compare March 29, 2019 23:27

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 29, 2019

srvaroa force-pushed the master branch from 8428521 to da08d0b Compare March 30, 2019 17:17

srvaroa force-pushed the master branch from da08d0b to 7097aca Compare April 7, 2019 15:08

daisy-ycguo reviewed Apr 9, 2019

View reviewed changes

srvaroa force-pushed the master branch from a37a612 to 8d0bf04 Compare April 9, 2019 17:34

srvaroa force-pushed the master branch 2 times, most recently from 53f355e to 979a56e Compare April 10, 2019 09:51

srvaroa force-pushed the master branch from 979a56e to 355ca41 Compare April 20, 2019 16:53

srvaroa force-pushed the master branch from 355ca41 to df3f51d Compare April 24, 2019 17:34

srvaroa force-pushed the master branch from df3f51d to dd11574 Compare May 3, 2019 07:51

srvaroa added 15 commits May 6, 2019 21:09

awssqs: move sample, clean docs

b21f81c

Signed-off-by: Galo Navarro <[email protected]>

awssqs: move config

87761f5

Signed-off-by: Galo Navarro <[email protected]>

awssqs: move adapter

438b435

Signed-off-by: Galo Navarro <[email protected]>

aswsqs: move controller

cc41687

Signed-off-by: Galo Navarro <[email protected]>

awssqs: clean stray codegens

c4b3557

Signed-off-by: Galo Navarro <[email protected]>

awssqs: use display, not dumper

d61ec79

Signed-off-by: Galo Navarro <[email protected]>

awssqs: README uses paths relative to root

0a853e9

Signed-off-by: Galo Navarro <[email protected]>

awssqs: be more precise about logs.

ae74151

Signed-off-by: Galo Navarro <[email protected]>

awssqs: fix review comments

d7061cb

Signed-off-by: Galo Navarro <[email protected]>

awssqs: fix path

decb569

Signed-off-by: Galo Navarro <[email protected]>

awssqs: fix rebase

0838c4d

Signed-off-by: Galo Navarro <[email protected]>

awssqs: fix VENDOR-LICENSE generation

a23aaa4

Signed-off-by: Galo Navarro <[email protected]>

awssqs: fix log message

a50b45f

Signed-off-by: Galo Navarro <[email protected]>

awssqs: update display-resources.yaml

3c70389

Signed-off-by: Galo Navarro <[email protected]>

awssqs: fixes in roles from review

948b497

Signed-off-by: Galo Navarro <[email protected]>

srvaroa force-pushed the master branch from d40eb6d to 948b497 Compare May 6, 2019 19:13

evankanderson reviewed May 8, 2019

View reviewed changes

awssqs: remove stray VerbAll

104e383

Signed-off-by: Galo Navarro <[email protected]>

evankanderson reviewed May 9, 2019

View reviewed changes

knative-prow-robot assigned evankanderson May 9, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 9, 2019

knative-prow-robot merged commit f62c238 into knative:master May 9, 2019

srvaroa mentioned this pull request May 10, 2019

awssqs: fix breakage after moving to contrib/ #406

Merged

matzew mentioned this pull request May 10, 2019

AWS is out in contrib, hence removing #408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move AWS SQS source to contrib #253

Move AWS SQS source to contrib #253

srvaroa commented Mar 16, 2019

srvaroa commented Mar 16, 2019

matzew Mar 18, 2019

srvaroa Mar 18, 2019

srvaroa Mar 20, 2019

evankanderson commented Mar 29, 2019

srvaroa commented Mar 29, 2019

srvaroa commented Mar 30, 2019

daisy-ycguo Apr 9, 2019

srvaroa Apr 9, 2019

srvaroa Apr 9, 2019 •

edited

Loading

daisy-ycguo Apr 9, 2019

srvaroa Apr 9, 2019

vaikas May 3, 2019

n3wscott commented Apr 9, 2019

srvaroa commented Apr 9, 2019

srvaroa commented Apr 9, 2019

grantr commented Apr 9, 2019

srvaroa commented Apr 10, 2019

srvaroa commented Apr 20, 2019

srvaroa commented Apr 24, 2019

srvaroa commented May 6, 2019

srvaroa commented May 6, 2019

evankanderson left a comment

evankanderson May 8, 2019

srvaroa May 8, 2019

knative-metrics-robot commented May 8, 2019

evankanderson left a comment

nachocano commented May 10, 2019

srvaroa commented May 10, 2019

nachocano commented May 10, 2019

Move AWS SQS source to contrib #253

Move AWS SQS source to contrib #253

Conversation

srvaroa commented Mar 16, 2019

srvaroa commented Mar 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Mar 29, 2019

srvaroa commented Mar 29, 2019

srvaroa commented Mar 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srvaroa Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n3wscott commented Apr 9, 2019

srvaroa commented Apr 9, 2019

srvaroa commented Apr 9, 2019

grantr commented Apr 9, 2019

srvaroa commented Apr 10, 2019

srvaroa commented Apr 20, 2019

srvaroa commented Apr 24, 2019

srvaroa commented May 6, 2019

srvaroa commented May 6, 2019

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented May 8, 2019

evankanderson left a comment

Choose a reason for hiding this comment

nachocano commented May 10, 2019

srvaroa commented May 10, 2019

nachocano commented May 10, 2019

srvaroa Apr 9, 2019 •

edited

Loading