-
Notifications
You must be signed in to change notification settings - Fork 67
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
Broker and source docs improvements #555
Broker and source docs improvements #555
Conversation
42d4626
to
10f63ae
Compare
Codecov Report
@@ Coverage Diff @@
## main #555 +/- ##
==========================================
- Coverage 75.58% 75.50% -0.09%
==========================================
Files 47 47
Lines 2892 2870 -22
==========================================
- Hits 2186 2167 -19
+ Misses 568 565 -3
Partials 138 138
Continue to review full report at Codecov.
|
f8d0ca4
to
8b61ea4
Compare
2ada76f
to
a014efb
Compare
source/README.md
Outdated
|
||
### Installation | ||
|
||
* Install the source from the nightly build: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be moved to the Development.md instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about getting read of the Development.md (maybe there are some things to keep) but with this readme and the samples I think everything should be covered
…example promoting the use of internal tools and with all the components needed to do a basic topology + set the prefetch count to use its default value to one but added comment on the file for the user to be able to modify it and understand whats going on
added some extra info and fixed typo fixed style error in yaml added some extra info and fixed typo, removing trailing spaces from source sample readme
a014efb
to
d65d6f2
Compare
d65d6f2
to
50ee0cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gabo1208 it's looking great :)
Added some minor comments and there are a few comments from before.
NAME URL AGE READY REASON | ||
default http://default-broker-ingress.dlq-demo.svc.cluster.local 2m39s True | ||
default http://default-broker-ingress.broker-trigger-demo.svc.cluster.local 2m39s True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing the formatting.
Thoughts on moving kubectl -n broker-trigger-demo get brokers
to after the Broker install ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be valuable for the samples to simplify the setup required to run them, so does it make sense to refer to the releases of the Broker and Source and remove references to building them from source code ?
…link + added source readme steps to build and install the source + added source example and fixed some typos and broken links
6bad5df
to
3ac8233
Compare
added newline at the end of the source sample file added newline to source dev md file fixed typo in source md
3ac8233
to
6a69856
Compare
readded table of contents to the source
changed development references to readme changed source readme formats
8f06cee
to
6d3d320
Compare
NAME URL AGE READY REASON | ||
default http://default-broker-ingress.dlq-demo.svc.cluster.local 2m39s True | ||
default http://default-broker-ingress.broker-trigger-demo.svc.cluster.local 2m39s True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with the headers
And i think here its ok cause since the ksvc is the broker's sink so the msg may change
### Cleanup | ||
|
||
```sh | ||
kubectl delete ns broker-trigger-demo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be the standard on knative docs to do the ns delete. I see what you mean but for all of the samples in this repo this work
### Cleanup | ||
|
||
```sh | ||
kubectl delete ns source-demo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
answered above
source/README.md
Outdated
|
||
### Installation | ||
|
||
* Install the source from the nightly build: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about getting read of the Development.md (maybe there are some things to keep) but with this readme and the samples I think everything should be covered
NAME URL AGE READY REASON | ||
default http://default-broker-ingress.dlq-demo.svc.cluster.local 2m39s True | ||
default http://default-broker-ingress.broker-trigger-demo.svc.cluster.local 2m39s True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the broker has a DLQ defined it could be not ready if we list it earlier
/lgtm |
Thanks for driving this @gabo1208 👍 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gabo1208, xtreme-sameer-vohra 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 |
Changes
/kind documentation
Fixes #553
Release Note
Docs