Skip to content

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

Closed
yarko wants to merge 3 commits into
openedx:masterfrom
yarko:yarko/fix-rake-env-assets
Closed

assets: add theme path to coffeescript's processing; rakefile: fix feature/env settings#2387
yarko wants to merge 3 commits into
openedx:masterfrom
yarko:yarko/fix-rake-env-assets

Conversation

@yarko

@yarko yarko commented Jan 31, 2014

Copy link
Copy Markdown
Contributor

This is another step towards enabling product development off of Open edX:

rakefile setup ENV_TOKENS (supposed to b django settings & feature flags) based on SERVICE_VARIANT, but the *.env.json files which are commonly set in the baseline are cms.env.json and lms.env.json - yet the feature settings weren't getting passed on to assets processing.

This corrects the base rakefile:

  • if SERVICE_VARIANT is set from the environment or call, it is honored.
  • if not, the rake command line ARGV is parsed for command specifiers indicating either lms or studio/cms;
  • if lms or studio is found, then the appropriate *.env.json file is setup for parsing, and passing on to tasks;

The standard installation creates both lms.env.json and cms.env.json in /edx/app/edxapp - copies of the respective django environment settings.

I didn't expect both to be present (for example: rake devstack[lms] or rake cms:gather_assets:devstack seem to preclude both lms and [studio, cms] being present on the command line. If there were such an instance, lms would win.

With this change, it is possible to effect lms theming and "have it just work".

There is a remaining issue with cms theming, where manage.py cms preprocess_assets seems to edit and overwrite the theme includ settings configured by other tasks in the various standard *.scss files. That will be addressed in another pull request.

This pull also enables automatic "working" for an associated pull [TBD] for configuration, where vagrant/base bringups are setup for specific ("nailed") repositories & versions (branches), so this is part of enabling that also.

@yarko

yarko commented Jan 31, 2014

Copy link
Copy Markdown
Contributor Author

@feanil @jarv - would you review? I've just tested this locally against focaccia/devstack, all looks good (am continuing some tests).

Thanks.

@yarko

yarko commented Jan 31, 2014

Copy link
Copy Markdown
Contributor Author

@nedbat - if you have the time to lay your eyes on it, I wouldn't mind that...

@yarko

yarko commented Jan 31, 2014

Copy link
Copy Markdown
Contributor Author

@jbau would you look at this also?

@jbau

jbau commented Jan 31, 2014

Copy link
Copy Markdown

@yarko I can take a look -- we've done basically no CMS theming work, FYI. Our studio is basically unthemed.

@yarko

yarko commented Jan 31, 2014

Copy link
Copy Markdown
Contributor Author

@jbau - I assumed as much; however, getting the theming working from ansible config means that cms / rake commands fail, so ultimately resolving every brick along this path is in my radar (see #2387)

@jbau

jbau commented Feb 2, 2014

Copy link
Copy Markdown

I see--so now for dev boxes that aren't using vagrant, we'd have lms.env.json and cms.env.json (instead of env.json) in the parent directory of edx-platform. This behavior unifies how dev and prod boxes are run, is that your intention?

@yarko

yarko commented Feb 2, 2014

Copy link
Copy Markdown
Contributor Author

@jbau - First, I don't have experience w/ production boxes (I'm trying to walk the path, one click at a time, from vagrant-devstack to vagrant-fullstack to prod-deploy). Yes, my intention is to unify (or smooth the path) between dev and prod. First step is just to get the basics working (i.e. if ansible extra vars are set, have them behave / be passed on in a normally expected way). Right now, this is simply trying to relieve that first roadblock.

I expect that the *.env.json files on vagrant devbox are created by ansible provisioning scripts. I would expect the same (ansible provisioning) to be used for non-vagrant dev-boxes (at least, it seems a reasonable expectation to me- I'm open to enlightenment). As it is, any extra-vars I set in either ansible-playbook, or the vagrant ansible provisioner gets passed, and copied out by something into the *.env.json. This is simply trying to complete the path for other parts of the system (e.g. rake) to also hear, be aware of those configuration settings (since rake tasks are acting / written "as if" they assume to have that knowledge).

Does production just have env.json only - no lms.env.json, no cms.env.jsn? If this is the case, then I should add a test to this pull to see if env.json, unadorned exists first, and only parse for one of the dev (?) variants when needed.

@jbau

jbau commented Feb 2, 2014

Copy link
Copy Markdown

Actually it's quite the opposite. On my dev machine, I only had env.json, but production has lms.env.json and cms.env.json. The env.json was handcrafted, but my dev env predates ansible provisioning, so that's why it's handcrafted.

If you're going to do any sort of precedence in importing, I'd put the [cms|lms].env.json higher than env.json.

@yarko

yarko commented Feb 3, 2014

Copy link
Copy Markdown
Contributor Author

@jbau - thanks for your input / insights. What I definitely want to avoid
is forcing people to type things like

rake devstack[lms] SERVICE_VARIANT=lms

which just seems silly. For handcrafted
env.json, requiring a SERVICE_VARIANT spec seems a little less troublesome,
but even then I'd prefer to see this all managed in one place: thru a
"handcrafted" --extra-args json sent to the ansible call once, and then
have every tool inside the dev/fullstack/deployed environment have access to the
feature, and other variants and settings. This seems cleaner to me. Let
me see what behavior SERVICE_VARIANT="" will cause, and think about it
(after all, having some prefix for handcrafted *.env.jsons doesn't
seem, to me, quite as ugly, going forwaard).

On Sun, Feb 2, 2014 at 5:00 PM, Jason Bau notifications@github.com wrote:

Actually it's quite the opposite. On my dev machine, I only had env.json,
but production has lms.env.json and cms.env.json. The env.json was
handcrafted, but my dev env predates ansible provisioning, so that's why
it's handcrafted.

If you're going to do any sort of precedence in importing, I'd put the
[cms|lms].env.json higher.

Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/2387#issuecomment-33916210
.

@yarko

yarko commented Feb 5, 2014

Copy link
Copy Markdown
Contributor Author

@yarko

yarko commented Feb 5, 2014

Copy link
Copy Markdown
Contributor Author

This is tested against current master (which works w/ stanford canonical theme); it needs a rebase, and then will be ready to merge.

@jarv @feanil - would you review?

@yarko

yarko commented Feb 5, 2014

Copy link
Copy Markdown
Contributor Author

On current master, I noticed rake devstack[lms] is now (!) complaining:

edxapp@precise64:~/edx-platform$ rake devstack[lms]
/edx/app/edxapp/edx-platform/rakefile:26: warning: already initialized constant SERVICE_VARIANT

@jarv - do I remember you putting in something recently to set default lms as SERVICE_VARIANT?

If so, note: rake needs more, other settings in /edx/app/edxapp/lms.env.json.

@ovnicraft

Copy link
Copy Markdown
Contributor

Hi @yarko @jbau ! what is the status of this ? we are working in edx theme, we want to integrated .. so this is the right way ?

@yarko

yarko commented Mar 8, 2014

Copy link
Copy Markdown
Contributor Author

@ovnicraft, @jbau - This is good, and I have used a lot, but it's partial. There are 3 more (4 in total) code PRs, and a documentation PR to go with this one - once I finish up the code on the last of the code ones (last one is close, the rest pretty much done), I will generate the those PRs, and start the documentation one - then open for comment and use. It should go through that first. Look for those in the next few days.

yarko added 2 commits March 11, 2014 00:44
rakefile: fix so assets/coffee use env vars & feature settings
@yarko

yarko commented Mar 11, 2014

Copy link
Copy Markdown
Contributor Author

rebased.
cms must be able to handle custom theme paths, so share same startup.py with lms.

@singingwolfboy

Copy link
Copy Markdown
Contributor

Now that Paver is finally, truly merged, is this pull request still relevant? Or should it be closed?

@yarko

yarko commented Mar 27, 2014

Copy link
Copy Markdown
Contributor Author

Closing this PR. As paver replaces rake functionality, so PR #3094 replaces this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants