-
Notifications
You must be signed in to change notification settings - Fork 11
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
SAM-209 organize documentation #452
Conversation
…EADME, moving extraneous README into docs, adding diagrams, etc. [SAM-209]
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 think overall this looks good.
We may want to move the documentation on PUML to somewhere more general but I think we can leave it in here for now.
Some of the figures seemed a little superfluous but I guess they are fine.
I did have a few minor remarks that we may want to correct first.
Also, I couldn't find a place to add the remark. I think the Kafka is more of a connection than a REST call. But I could be mistaken.
_temp/system-context.puml
Outdated
@@ -0,0 +1,53 @@ | |||
@startuml system-context |
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 see _temp in the gitignore but these are here. Is that intentional?
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.
Uh, yeah. Let me clean that up ...
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 think overall this looks good.
We may want to move the documentation on PUML to somewhere more general but I think we can leave it in here for now.
Some of the figures seemed a little superfluous but I guess they are fine.
I did have a few minor remarks that we may want to correct first.
Also, I couldn't find a place to add the remark. I think the Kafka is more of a connection than a REST call. But I could be mistaken.
I did a quick check on the kafka client, and it did appear that it at least does support a REST api, whether the python client uses that or not I don't know. In the end, I think the most important attribute of the overall architecture diagram is to show the major components and their inter-dependencies. More detail, accurate, is also useful. Tried to keep in mind people who are and aren't familiar with the project.
Also, in the end, the purpose of this this diagram in this PR is more exemplary and can certainly be improved. More diagrams are required to capture greater detail for specific areas (e.g. how kafka interacts with it, how sample service interacts with RE, the interaction of import with sample service), and some diagrams may be out of context for the sample service in isolation, and should be in a sample "system" documentation repo.
|
||
# Installation from another module | ||
|
||
To use this code in another SDK module, call `kb-sdk install SampleService` in the other module's root directory. |
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.
We may need to update this since it isn't a dynamic service any longer and this could result in getting an old client.
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.
Yes, I left that and some other old docs in there. I did try to fix some clearly obsolete information. In this case I wasn't sure if it was still valid to use kb-sdk to install a client, or if that is the most common practice, compared to using a generic client, so left that in place. It can certainly be removed at any time if it is invalid.
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.
Also, I didn't read whole swaths of existing content for accuracy or completeness, just moved them around.
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 see the release notes in the docs directory. I think I would prefer to keep those at the top. I see it is empty so maybe the intent was something else.
That was intended to be a document about creating release notes, as it is located in the documentation directory of the docs directory. I've updated the empty document to have a title and a quote-blocked reminder to complete the content. There is already a Interestingly, I've found that some Python projects locate release notes within the documentation folder, not at the project root. I found this a bit baffling at first, but it does make some sense, as it could be argued that for a documentation set to be complete it should include a description of the evolution of the product over time, including release notes. |
Resolved a new merge conflict introduced by concurrent changes to RELEASE_NOTES.md |
This set of changes is designed to satisfy SAM-209.
It attempts to do so by:
/docs
directoryREADME.md
into separate documentsREADME.md
according to https://github.com/RichardLitt/standard-readmeIn addition, I've cherrypicked a few new documents from SAM-96, from which this work was initially copied. This is because this set of changes should also satisfy the needs of the changes for SAM-96, which needs to add several documents and diagrams. Separate from SAM-209, it was requested to remove the document reorganization from SAM-96, so adding these additional changes here allows us to base those changes on this work.