-
Notifications
You must be signed in to change notification settings - Fork 140
Add "Advanced usage" to readme #338
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
base: main
Are you sure you want to change the base?
Conversation
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.
Thank you for the contribution! I have several suggestions to improve the documentation:
- Missing rebuild instructions
The current example shows changing the code and then running the container, but it's missing the rebuild step:
- To get more than a minimal example, change the code and then run the container
- To get more than a minimal example, change the code, rebuild the image, and then run the container
You should add the rebuild command before running:
docker build -t my-selenium-lambda .
docker run --name selenium-lambda-demo -p 9000:8080 my-selenium-lambda
- Add language specifiers to code blocks
All code blocks should have language specifiers for proper syntax highlighting:
- Shell commands: ```bash
- Python code: ```python
- Use permalink for code reference
Instead of:
Change https://github.com/umihico/docker-selenium-lambda/blob/main/main.py#L25
Use a permalink to a specific commit to prevent broken links when the file changes:
Change https://github.com/umihico/docker-selenium-lambda/blob/7a19acc/main.py#L25
- Container naming
The container name "demo" is too generic. Consider using something more specific like:
- selenium-lambda-demo
- aws-lambda-selenium-demo
- Serverless framework example verification
Has the serverless framework example been tested?
sls invoke --function demo --data '{"url": "https://github.com"}'
Please confirm this works with the current setup.
Done but regarding 5. I must admit, I never used sls in my life😅I tested everything else though. Maybe you can test it or I'll try doing it later |
Then please just remove the step. Unconfirmed commands should never be on the README.. BTW, did you confirm others? |
I had a small typo because my intention in #337 was to put "target" in the event, otherwise everything works 💪 |
Is there anything else that you'd like to change @umihico ? |
"Fixes" #337