ensure SERVICE_VARIANT set; pass theme to coffeescript; #3094

yarko · 2014-03-27T23:38:41Z

This enables theming with paver (PR #3075), mirroring similar changes as for rake in PR #2387.

In particular, when the following are set:

edxapp_theme_name: stanford
edxapp_theme_source_repo: https://{{ COMMON_GIT_MIRROR }}/{{ THEME_ACCT }}/edx-theme.git
edxapp_theme_version: master
edxapp_use_custom_theme: true  # false to disable & use std. theme

then:

paver devstack lms

will correctly read and pass the paths to both coffee and sass.

Alternately, the process described in the comment of https://gist.github.com/yarko/8818633 is another way to test this - set the above four theme settings in build-version.yml, then run vagrant provision.

A way to run this on vagrant fullstack is to use build-version.yml, as above, and either include it in, or symlink it to /edx/app/edx_ansible/server-vars.yml (it shows up as /vagrant/build-version.yml), and then using update edx-platform some-version.

Note: currently you will need to completely remove edx-platform in order to clone one from a different repository (since git remotes is not managed thru update).

This PR corrects two deficiencies:

no SERVICE_VARIANT set:
prevents tasks from reading the appropriate /edx/app/edxapp *.env.json file;
FIX (pavelib/utils/env.py):
- parse command line args for one of 'lms' or 'cms' or 'studio', and set SERVICE_VARIANT appropriately;
coffeescript path not update with theme (once vars read from *.env.json):
FIX (pavelib/assets.py):
- add theme path, when appropriate, to the search path for coffee;

Dubious code:

Please review the following aspects:

env.py: class variables set at class level (not instantiation level), because of current use, e.g. Env.REPO_ROOT
- if the use case exists of a single command line instance of paver, with multiple tasks then this could confound proper operation;
- I rather prefer having None base class attributes, and setting them in __init__(), since this would ensure proper operation. I did not follow this path because it would have meant more code changes (no Env.REPO_ROOT anywhere), and would add instantiation of the class. I prefer consensus on the approach first (so took the path closer to the current code).
assets.py:
- for no particularly good reason, THEME_COFFEE_PATHS and THEME_SASS_PATHS set at module level (not w/in function); not sure I like this.

Goal

My goal and intent is to have Vagrantfile and update both honor build-version.yml files as extra-vars. At that point, a developer's Getting Started guide to Open edX will be an easier thing to write. This will simplify getting started, and maintaining edx instances, since common configurations will not require "deep" repository involvement. Build-version.yml is consistent with and distinct from server-vars.yml (server configuration and settings).

This goal will involve a PR in the configuration repository (w.i.p.) to update the various Vagrantfile instances, and the update script template.

Would you please review, @jzoldak & @singingwolfboy

singingwolfboy · 2014-03-28T16:43:10Z

pavelib/utils/envs.py

+        # consider only words remaining (parse out separators)
+        p = re.compile(r'\W+')
+        clean_args = p.split(' '.join(clean_args))
+        # if lms or cms is among command line words, use it's env


If your goal here is simply to detect if "lms" or "cms" is one of the arguments, why not do something like if "lms" in sys.argv or if "cms" in sys.argv?

Good golly! Because I did a quick port from the rake version?
:-/
Updating PR now...

jzoldak · 2014-03-28T17:54:56Z

@yarko I won't be able to get to this today and I'll be out on Monday and Tuesday. Just to let you know.
@singingwolfboy thx for jumping on this!

yarko · 2014-03-28T17:56:54Z

👍 thanks @jzoldak

singingwolfboy · 2014-03-31T17:38:37Z

pavelib/utils/envs.py

+    if not SERVICE_VARIANT:  # this will intentionally catch "";
+        SERVICE_VARIANT = 'lms' if 'lms' in sys.argv[1:] \
+                    else 'cms' if ('cms' in sys.argv[1:]) or ('studio' in sys.arg[1:]) \
+                    else SERVICE_VARIANT


This is way too much logic in one line of code. Can you split this into multiple lines, so it's easier to read?

sure ... 3 lines; 4 lines; same logic, different form... not much difference to me - try this on for size (yeah, reads a little nicer - I like the "any").

singingwolfboy · 2014-04-01T13:31:59Z

@yarko Looks good, but this PR now has conflicts and needs a rebase. :(

yarko · 2014-04-01T16:46:04Z

no worries - will get to it after lunch.

rebased / squashed now.

yarko · 2014-04-08T16:25:15Z

@singingwolfboy @jzoldak - did you get a chance to look at this?

singingwolfboy · 2014-04-08T17:10:05Z

Thanks for the ping! This looks good to me, but I'd like to get someone from Stanford to test this as well, since this affects their theming capabilities. Maybe @sefk?

yarko · 2014-04-08T17:40:37Z

@sefk, @caesar2164 - would you have a look? - @caesar2164, I know you once had pending changes (Stanford-Online/edx-theme#4).

(sorry - previous comment appeared double-posted;)

caesar2164 · 2014-04-08T18:29:16Z

@yarko - how do I run your branch? It breaks for me with our theme settings set...

yarko · 2014-04-15T08:06:16Z

@caesar2164 - sorry, was a busy during PyCon; just rebased again. See comment on https://gist.github.com/yarko/10711787 for how I use those files to test.

Note: the test uses my fork of Stanford-Online/edx-theme#4; recall you had other changes in the wings;

/cc @sefk @singingwolfboy

caesar2164 · 2014-04-17T20:51:13Z

@yarko - this works great for me, 👍! (@sefk also wants to look at it…)

sefk · 2014-04-17T21:18:25Z

pavelib/assets.py

@@ -131,7 +128,7 @@ def compile_sass(debug=False):
    """
    Compile Sass to CSS.
    """
-    theme_paths = theme_sass_paths()
+    theme_paths = THEME_SASS_PATHS


The only reason for this local variable before was to keep from calling theme_sass_paths() twice. Now that you're using the THEME_SASS_PATHS list, you may as well just use in the two join()'s below and avoid the extra variable. That would be cleaner and more consistent.

sefk · 2014-04-17T21:23:17Z

I've tested this as well @yarko and indeed this is a helpful change. It's a bit hacky in that I still don't understand why SERVICE_VARIANT matters with themed assets and not just "normal" assets, but that's OK. Much better to do this change and remove the landmine than to try to untangle that harder issue.

I tested this on my local dev env (devstack) and it works nicely, where master didn't before.

Aside from my one little variable nit above, this LGTM and I'd be in favor of pulling in. 👍

yarko · 2014-04-18T15:59:35Z

@sefk - re: your THEME_SASS_PATHS line comment: yes, I agree (I think originally, I wanted to show how little the change was for reviewers).

I'll make that change, and rebase in a few hrs. Then ask @jzoldak & @singingwolfboy to test / merge.

…ssed to coffeescript also;

jzoldak · 2014-04-18T20:22:14Z

@yarko I added you to the whitelist on jenkins so that PRs from your fork will automatically run the tests when you push commits.

yarko · 2014-04-18T20:50:28Z

Ok - rebased, @sefk suggested change made.
@jzoldak @singingwolfboy @caesar2164 - good to go? Once tests pass, can we get this merged?

caesar2164 · 2014-04-18T20:54:01Z

@yarko - 👍 from me once tests pass.

jzoldak · 2014-04-18T21:05:13Z

pavelib/utils/envs.py

+         args = sys.argv[1:]
+         if 'lms' in args:
+             SERVICE_VARIANT = 'lms'
+         elif any(i in args for i in ('cms', 'studio')):


Commands are failing with: NameError: global name 'args' is not defined

E.g. paver --help which should print the list of available tasks. This fails for me on devstack with no SERVICE_VARIANT set.

Same thing is happening when tests are kicked off by jenkins.

…"paver --help" work cleanly)

yarko · 2014-04-19T01:06:59Z

@jzoldak @singingwolfboy @sefk @caesar2164 - tests passed; I think this is ready now.

Can we merge?

ensure SERVICE_VARIANT set; pass theme to coffeescript;

This was referenced Mar 27, 2014

assets: add theme path to coffeescript's processing; rakefile: fix feature/env settings #2387

Closed

link studio startup.py to lms/startup.py #3095

Closed

nedbat added the open-source-contribution label Mar 28, 2014

singingwolfboy reviewed Mar 28, 2014
View reviewed changes

singingwolfboy reviewed Mar 31, 2014
View reviewed changes

sefk reviewed Apr 17, 2014
View reviewed changes

set reasonable SERVICE_VARIANT based on command line; ensure theme pa…

59106f2

…ssed to coffeescript also;

remove theme_paths var, per @sefk

cc86b65

jzoldak reviewed Apr 18, 2014
View reviewed changes

fix class var; set SERVICE_VARIANT to lms by default if unset (helps …

97ecc7a

…"paver --help" work cleanly)

singingwolfboy added a commit that referenced this pull request Apr 19, 2014

Merge pull request #3094 from yarko/yarko/fix-paver-env-assets

b44ed80

ensure SERVICE_VARIANT set; pass theme to coffeescript;

singingwolfboy merged commit b44ed80 into openedx:master Apr 19, 2014

baby636 mentioned this pull request Jan 10, 2023

[Snyk] Fix for 1 vulnerabilities baby636/edx-platform#43

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ensure SERVICE_VARIANT set; pass theme to coffeescript; #3094

ensure SERVICE_VARIANT set; pass theme to coffeescript; #3094

yarko commented Mar 27, 2014

singingwolfboy Mar 28, 2014

yarko Mar 28, 2014

jzoldak commented Mar 28, 2014

yarko commented Mar 28, 2014

singingwolfboy Mar 31, 2014

yarko Apr 1, 2014

singingwolfboy commented Apr 1, 2014

yarko commented Apr 1, 2014

yarko commented Apr 8, 2014

singingwolfboy commented Apr 8, 2014

yarko commented Apr 8, 2014

caesar2164 commented Apr 8, 2014

yarko commented Apr 15, 2014

caesar2164 commented Apr 17, 2014

sefk Apr 17, 2014

sefk commented Apr 17, 2014

yarko commented Apr 18, 2014

jzoldak commented Apr 18, 2014

yarko commented Apr 18, 2014

caesar2164 commented Apr 18, 2014

jzoldak Apr 18, 2014

yarko commented Apr 19, 2014

ensure SERVICE_VARIANT set; pass theme to coffeescript; #3094

ensure SERVICE_VARIANT set; pass theme to coffeescript; #3094

Conversation

yarko commented Mar 27, 2014

This PR corrects two deficiencies:

Dubious code:

Goal

singingwolfboy Mar 28, 2014

Choose a reason for hiding this comment

yarko Mar 28, 2014

Choose a reason for hiding this comment

jzoldak commented Mar 28, 2014

yarko commented Mar 28, 2014

singingwolfboy Mar 31, 2014

Choose a reason for hiding this comment

yarko Apr 1, 2014

Choose a reason for hiding this comment

singingwolfboy commented Apr 1, 2014

yarko commented Apr 1, 2014

yarko commented Apr 8, 2014

singingwolfboy commented Apr 8, 2014

yarko commented Apr 8, 2014

caesar2164 commented Apr 8, 2014

yarko commented Apr 15, 2014

caesar2164 commented Apr 17, 2014

sefk Apr 17, 2014

Choose a reason for hiding this comment

sefk commented Apr 17, 2014

yarko commented Apr 18, 2014

jzoldak commented Apr 18, 2014

yarko commented Apr 18, 2014

caesar2164 commented Apr 18, 2014

jzoldak Apr 18, 2014

Choose a reason for hiding this comment

yarko commented Apr 19, 2014