-
Notifications
You must be signed in to change notification settings - Fork 5
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 Python server #209
Add Python server #209
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.
Is this already deployed?
No, the infra project is pulling from this branch. |
This probably should be put in the infra repository than this one. I say this because this has not very much to do with the actual Firebase / open source project, but a lot to do with the AWS / Cosmos side of things. |
* Bump react-dev-utils from 11.0.3 to 11.0.4 (#193) Bumps [react-dev-utils](https://github.com/facebook/create-react-app/tree/HEAD/packages/react-dev-utils) from 11.0.3 to 11.0.4. - [Release notes](https://github.com/facebook/create-react-app/releases) - [Changelog](https://github.com/facebook/create-react-app/blob/master/CHANGELOG-1.x.md) - [Commits](https://github.com/facebook/create-react-app/commits/HEAD/packages/react-dev-utils) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Deal with unhandled exceptions in Admin view (#204) * Deal with unhandled exceptions in Admin view. [fix] add default params to Transcripts view. Unsure when the bug was introduced. Also had to change the admin role - not sure when this was changed either. * [fix] Actually should have a better error message * [fix] Better message * refactor(env): #198 Add logic to handle both test and live envs at build Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Eimi Okuno <[email protected]>
This is true but it's a simple, generic server with no BBC-specific config so I'm happy for it to belong in this repo. |
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 to me, Tamsin - hero status for your fixes with the lets / consts!
src/Util/is-production/index.js
Outdated
const isProduction = () => { | ||
const parsedUrl = url.parse(window.location.href, true); | ||
|
||
if ((parsedUrl.host) === 'digital-paper-edit.tools.bbc.co.uk') { |
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.
Could this be simplified? e.g.
return parsedUrl.host === 'digital-paper-edit.tools.bbc.co.uk'
or does this make it less clear?
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 great and works for me! Just had one comment that may not need addressing!
import http.server | ||
import socketserver | ||
class MyHttpRequestHandler(http.server.SimpleHTTPRequestHandler): | ||
import SimpleHTTPServer, SocketServer |
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.
Does this need to use python2? Why is that?
Using python3 I had to change it back to:
import http.server, socketserver
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.
Good q - the EC2 we use has python 2 by default (as does my Mac :) )
Don’t understand the need for an external dependency on url when window.location.host already provides that value?
On 28 Apr 2021, at 15:55, Sarah Rainbow ***@***.***> wrote:
@sarahrainbow commented on this pull request.
________________________________
In src/Util/is-production/index.js<#209 (comment)>:
@@ -0,0 +1,14 @@
+import url from 'url';
+
+const isProduction = () => {
+ const parsedUrl = url.parse(window.location.href, true);
+
+ if ((parsedUrl.host) === 'digital-paper-edit.tools.bbc.co.uk') {
Could this be simplified? e.g.
return parsedUrl.host === 'digital-paper-edit.tools.bbc.co.uk'
or does this make it less clear?
|
Is your Pull Request request related to another issue in this repository ?
bbc/digital-paper-edit-infrastructure#21
Describe what the PR does
const
variables tolet
where reassignment was causing read-only errors when deployedState whether the PR is ready for review or whether it needs extra work
RFR
How to test
npm run build
cd build
python server.py