-
Notifications
You must be signed in to change notification settings - Fork 265
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
PHPLIB-1163 Create tutorial for using MongoDB with Bref #1273
Conversation
d8a655c
to
679b311
Compare
Co-authored-by: Andreas Braun <[email protected]> Co-authored-by: Jeremy Mikola <[email protected]>
Co-authored-by: Jeremy Mikola <[email protected]>
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 aside from phpcs complaints about the snake_case property name which we can ignore. I'll defer to @jmikola for a final look.
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.
You can sort out the versionadded
thing, but I'll defer to docs for the remainder of review.
docs/tutorial/aws-lambda.txt
Outdated
|
||
|
||
Let's try to use the MongoDB driver with this simple web page that list planets | ||
from the :manual:`sample dataset </atlas/sample-data/>`. |
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.
@ccho-mongodb: Is this the correct prefix?
The intended URL is https://www.mongodb.com/docs/atlas/sample-data/, but conf.py in mongodb/docs-php-library suggests this will be "http://docs.mongodb.org/manual%s".
Now that I'm thinking about it, that pattern looks incorrect since all docs should be pointing to a mongodb.com
domain. There's also the omission of "https".
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.
Indeed, I got it wrong. The prefix should be added. Maybe added to this standing PR: mongodb/docs-php-library#57
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.
Reverted to full url links for the time being.
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 the extlinks entry may override the Snooty role. The Docs Platform team can probably confirm/deny this.
For example, docs-java
doesn't have a conf.py
file and therefore no extlinks, but uses the :manual:
role:
https://github.com/mongodb/docs-java/blob/612975d4e0a2b550a9eb486b75558afb6ee7c55b/source/usage-examples/watch.txt#L181-L183
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, I asked about this in Slack and will revise mongodb/docs-php-library#57 based on the response.
docs/tutorial/aws-lambda.txt
Outdated
AWS sets environment variables that contains the access token and secret token with | ||
the role assigned to deployed function. | ||
|
||
Set up :manual:`unified AWS Access </atlas/security/set-up-unified-aws-access/>`: |
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.
@ccho-mongodb: Intended link here is https://www.mongodb.com/docs/atlas/security/set-up-unified-aws-access/
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 you can use the :atlas:
role unless it's overridden by extlinks:
https://github.com/mongodb/snooty-parser/blob/main/snooty/rstspec.toml#L1290-L1292
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.
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 you can use the :atlas: role unless it's overridden by extlinks:
Does the very existence of extlinks
in conf.py
prevent us from using any of the roles defined in main/snooty/rstspec.toml?
mongodb/docs-php-library#57 was going to introduce atlas
to extlinks
, but it's presently not there.
As I mentioned in Slack, we'd need a custom role for :php:
to link to PHP.net pages. I'm not a bit worried that reducing conf.py
to
extlinks = {
'php': ('https://php.net/%s', '')
}
...might mean we lose access to :manual:
as well.
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.
Disregard my last comment. @schmalliso confirmed that conf.py
is ignored by Snooty. All of the roles we need are already defined in main/snooty/rstspec.toml (including :php:
).
@GromNaN: I'm not sure why :atlas:
isn't working. Perhaps there's some other configuration where specific roles from rstspec.toml
need to be enabled for a project? This seems like a question for #ask-docs-platform
.
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.
Looks like a good start. I left a number of changes that I think would help our audiences understand the material more quickly. Let me know if you'd like me to take another look after making changes.
docs/tutorial/aws-lambda.txt
Outdated
:language: php | ||
|
||
|
||
Deploy the application |
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.
Issue:
This seems to be missing punctuation.
If this is meant to be the title of a step, maybe this tutorial can be formatted as steps.
Here's an example:
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.
@mongodb/dbx-php What's your opinion on the formatting of this tutorial? It's actually steps.
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.
No objections if you feel it's worth the effort.
Please make sure that formatting doesn't clash with "AWS Credentials", which has a numbered list within. I'm not expecting any issue nesting a list within the RST step syntax -- just a request to confirm the rendered output looks presentable.
Edit: looking at the Kafka tutorial, I consider it less useful that the steps aren't incorporated into the "On this page" navigation. Given that, I'd lean toward no changes.
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 skipping steps formatting is fine and more efficient with time. The issue I was focusing on was missing punctuation. Perhaps it could be something to experiment with in a future PR!
Review tutorial
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 @ccho-mongodb for your complete review. I did all the suggested changes and improved some wording.
Also updated the index.php
to correctly display errors messages.
docs/tutorial/aws-lambda.txt
Outdated
:language: php | ||
|
||
|
||
Deploy the application |
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.
@mongodb/dbx-php What's your opinion on the formatting of this tutorial? It's actually steps.
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 followup suggestions.
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!
Co-authored-by: Chris Cho <[email protected]>
* v1.18: (50 commits) Enable auto-merge in merge-up workflow (#1295) PHPLIB-1447: Add SBOM lite (#1292) Fix syntax error in docs (#1285) PHPLIB-1163 Create tutorial for using MongoDB with Bref (#1273) (#1282) Create sarif report when running psalm (#1280) Update composer.json and CI matrices for 1.18.0 PHPLIB-1410: Invoke drivers-evergreen-tools scripts with bash (#1267) PHPLIB-1302: Use Composer\InstalledVersions (#1262) PHPLIB-1320: Support "comment" command option in Collection::createIndex() (#1263) PHPLIB-1413: Use env instead of matrix for driver-version (#1261) Fix Markdown heading PHPLIB-1399: Docs examples for agg expr projection (#1260) PHPLIB-1412: Skip range encryption tests on MongoDB 8.0+ (#1259) PHPLIB-1409: Skip $out and mapReduce tests on serverless (#1254) Exclude read-write-concern tests from serverless testing (#1253) PHPLIB-1409: Convert default write concern tests to unified test format (#1252) PHPLIB-1408: Convert ADL spec test to unified test format (#1250) Remove redundant annotations (#1251) PHPLIB-1404: Convert retryable reads spec tests to unified test format (#1247) DOCSP-36627: Additional double backslash fixes for master (#1246) ...
* v1.19: DOCSP-37027: Fix build errors (#1289) PHPLIB-1415 Add tests for PHP 8.4 (#1287) PHPLIB-1163 Create tutorial for using MongoDB with Bref (#1273) Relax branch-alias on dev-master to avoid update on each release (#1277) PHPLIB-1424: Fix potentially racy w:0 unified tests (#1276) Leverage PHP 8.0 string functions (#1274) Master is now 1.19-dev
Fix PHPLIB-1163
The AWS lambda tutorial as 2 parts: