Skip to content
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

Serve static pages from app.yaml #6974

Merged
merged 22 commits into from
Jul 6, 2019
Merged

Conversation

jameesjohn
Copy link
Contributor

Explanation

Following the work on the static pages, this PR creates a new render_static_template method to serve the jinja-less templates

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The linter/Karma presubmit checks have passed.
    • These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
  • The PR is made from a branch that's not called "develop".
  • The PR has an appropriate "CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
  • The PR follows the style guide.
  • The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
  • The PR is assigned to an appropriate reviewer.
    • If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
    • If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue. Do not only request the review but also add the reviewer as an assignee.

Copy link
Contributor

@vojtechjelinek vojtechjelinek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Two important comments!

core/controllers/base.py Outdated Show resolved Hide resolved
core/controllers/base.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 19, 2019

Codecov Report

Merging #6974 into develop will decrease coverage by 0.02%.
The diff coverage is 6.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #6974      +/-   ##
===========================================
- Coverage    97.54%   97.52%   -0.03%     
===========================================
  Files          379      379              
  Lines        63366    63354      -12     
===========================================
- Hits         61809    61783      -26     
- Misses        1557     1571      +14
Impacted Files Coverage Δ
main.py 87.61% <ø> (ø) ⬆️
core/controllers/base_test.py 98.33% <ø> (ø) ⬆️
core/controllers/pages.py 100% <ø> (ø) ⬆️
core/controllers/pages_test.py 100% <ø> (ø) ⬆️
scripts/build.py 76.54% <6.66%> (-2.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8280c3...33c6f79. Read the comment docs.

@oppiabot
Copy link

oppiabot bot commented Jun 21, 2019

Hi @Jamesjay4199. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

Copy link
Contributor

@vojtechjelinek vojtechjelinek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just one question.

core/controllers/base.py Outdated Show resolved Hide resolved
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of things, happy to approve after that.

core/controllers/base.py Outdated Show resolved Hide resolved
core/controllers/base.py Outdated Show resolved Hide resolved

path = os.path.join(
feconf.FRONTEND_TEMPLATES_DIR, filepath)
self.response.write(template.render(path, None))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering ... what does template.render do? Does it have any unnecessary overhead? Is there a reference link you can point to that says "this is the preferred way to serve static files"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is "the preferred way" to serve static files, but this is what was in the documentation and I expect that since it comes bundled with webapp, then it should have been optimized for the purpose.
https://webapp2.readthedocs.io/en/latest/tutorials/gettingstarted/templates.html

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

template.render is similar to rendering with jinja, but it uses Django templating instead, so I'm not sure it's what you want to do here if your aim is to serve the files statically.

If you do want to serve a file statically, then typically that is done in app.yaml -- see https://cloud.google.com/appengine/docs/standard/python/config/appref

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(You might also want to try the three ways -- jinja render, django template.render, and app.yaml static rendering -- and compare their times.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

app.yaml - 42ms
Jinja render - 147ms
template.render - 119ms

So we should go with app.yaml, but how about the headers?

@seanlip seanlip assigned jameesjohn and unassigned seanlip Jun 23, 2019
@seanlip
Copy link
Member

seanlip commented Jun 23, 2019 via email

@jameesjohn
Copy link
Contributor Author

@seanlip, while implementing this, I noticed that it wouldn't work well for the other variants of splash page.
and also the X-Frame-Options header needs some script before getting set

@seanlip
Copy link
Member

seanlip commented Jun 23, 2019

Can you be more descriptive about what "wouldn't work well" means? Otherwise it is hard to offer advice.

For X-Frame-Options, I think it's usually DENY except for the exploration pages. Is that correct? If so you can just go with that...

@jameesjohn
Copy link
Contributor Author

Other splash pages like splash_at0 and splash_at1
they are rendered based on the value of c from the url

@jameesjohn
Copy link
Contributor Author

I'll just render them individually

@seanlip
Copy link
Member

seanlip commented Jun 23, 2019

Yep, sg.

@jameesjohn jameesjohn assigned aks681 and unassigned vojtechjelinek Jun 30, 2019
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Jamesjay4199, are there e2e tests for each of these pages? If so, could you please point me to them? I think we should add a note in build.py or app.yaml to say that each of these pages needs to be tested in an e2e test, too (in case future devs add stuff).

scripts/build.py Outdated Show resolved Hide resolved
scripts/build.py Outdated Show resolved Hide resolved
@@ -93,6 +93,27 @@ describe('Oppia static pages tour', function() {
waitFor.pageToFullyLoad();
});

it('visits the Get started page', function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seanlip, the tests are here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you assert for something that should exist in the page (to make sure it's not serving a 404)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

scripts/start.sh Outdated
@@ -89,10 +89,12 @@ if [[ "$FORCE_PROD_MODE" == "True" ]]; then
constants_env_variable="\"DEV_MODE\": false"
sed -i.bak -e s/"\"DEV_MODE\": .*"/"$constants_env_variable"/ assets/constants.js
$PYTHON_CMD scripts/build.py --prod_env --enable_watcher
APP_YAML="app.yaml"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APP_YAML_FILEPATH, here and elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -93,6 +93,27 @@ describe('Oppia static pages tour', function() {
waitFor.pageToFullyLoad();
});

it('visits the Get started page', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you assert for something that should exist in the page (to make sure it's not serving a 404)?

@@ -111,7 +111,12 @@
'third_party/generated/js/third_party.min.js.map',
'third_party/generated/webfonts/*',
'*.bundle.js',
'*.bundle.js.map')
'*.bundle.js.map',
'*/dist/get-started-page.mainpage.html',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the comment to explain why these files are present.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

app.yaml Outdated
@@ -2,9 +2,6 @@ runtime: python27
api_version: 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per our discussion, let's just have one app.yaml file in the codebase so that there's a single source of truth.

@vojtechjelinek FYI we discussed having a template file from which to generate dev/prod, but then we figured that it would be better to just have the "dev" version in the codebase (as we do now) and then do the changes needed to transform it to the "prod" version (like we do with other files/folders). This means that we only have one indirectly-generated file rather than two (which is better, since indirect pipelines like this often lead to errors).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seanlip, should this file still be in the codebase?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so.

@seanlip seanlip assigned apb7 and unassigned aks681 Jul 4, 2019
Copy link
Contributor Author

@jameesjohn jameesjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seanlip PTAL. Thanks

Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. I think just removing the app.yaml file suffices. Tganks!

@jameesjohn
Copy link
Contributor Author

Done

@seanlip
Copy link
Member

seanlip commented Jul 5, 2019

@Jamesjay4199 Don't forget to revert changes in package-lock.json if they are not intentional.

@apb7 PTAL as codeowner, thanks!

@seanlip
Copy link
Member

seanlip commented Jul 5, 2019

Also @Jamesjay4199 please note that the lint and an e2e test is failing.

Copy link
Contributor

@apb7 apb7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from code owner's perspective. Thanks!

@apb7 apb7 removed their assignment Jul 5, 2019
@jameesjohn
Copy link
Contributor Author

@seanlip, I think this is ready to be merged

Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, looks good. Thanks!

@seanlip seanlip merged commit 82d1217 into oppia:develop Jul 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants