-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add examples from MongoDB Docs #7
Add examples from MongoDB Docs #7
Conversation
PR Hints:
The areas I think deserve the most attention in this review are as follows: |
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.
First, this is this ticket, correct? https://jira.mongodb.org/browse/DOCSP-19236
I didn't see it linked in the description and am trying to get some context. Here are some questions that might steer any further feedback:
- Is this a repository that we're going to have documentation readers clone?
- Are we expecting them to run the build.py script or is that only for docs writers?
- I only looked at build.py, docs-examples/unsupported/README.md, and docs-examples/README.md as I wasn't able to decipher what "Fix issue where identical image names made it difficult to quickly switch between different examples" meant on Slack. Is there anything else I should take a look at?
Hi @ccho-mongodb, thanks for the feedback. I ran the code through
Apologies for being unclear. What I meant by "Fix issue where identical image names made it difficult to quickly switch between different examples" relates to the following scenario:
By giving the images unique to an activity names specific to that activity, Docker rebuilds images that differ between examples when switching between examples without needing to prune images. |
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.
LGTM with a few comments and nitpicks.
I think you should not spend much time with fiddling with the build.py script. Eventually, we should probably replace it with a shell script or other mechanism to remove the unnecessary dependency on python.
Whoops looks like there is a setting on this repo that automatically dismisses reviews if a significant amount of the content changes after the review takes place. Apologies, you may have to leave another approving review @ccho-mongodb. I'll ask @RWaltersMA to disable this feature as it does not seem to be present on our other repos. |
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.
LGTM, left a couple comments. To reiterate my comment from before, we should spend a minimal amount of time/effort on the python script and the pseudo-template mechanism.
"v1.0": {IS_SUPPORTED:False, CONFLUENT_VERSION:None} | ||
} | ||
|
||
def recursively_find_replace(directory, find, replace, file_pattern): | ||
'''Recursively find and replace a string in a directory.''' | ||
'''Recursively searches directories for all occurrences of a string and replaces them.''' |
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.
This looks good, but the rest of the function comments don't follow the recommended form yet.
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.
updated remaining function doc-strings
docs-examples/scripts/build.py
Outdated
"v1.3": {IS_SUPPORTED:False, CONFLUENT_VERSION:"1.3.0"}, | ||
"v1.2": {IS_SUPPORTED:False, CONFLUENT_VERSION:"1.2.0"}, | ||
"v1.1": {IS_SUPPORTED:False, CONFLUENT_VERSION:"1.1.0"}, |
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.
To reiterate my prior feedback, this isn't necessary because the info is not used. The dict you had originally just mapping named version to confluent version or None was fine and easier to understand (e.g. "v1.6": "1.6.1").
Add versioned examples from the Kafka IA-v2 docs set.
JIRA: https://jira.mongodb.org/browse/DOCSP-19236