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

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

Closed
wants to merge 3 commits into from

Conversation

yarko
Copy link
Contributor

@yarko yarko commented Jan 31, 2014

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
Copy link
Contributor Author

yarko commented Jan 31, 2014

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

Thanks.

@yarko
Copy link
Contributor Author

yarko commented Jan 31, 2014

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

@yarko
Copy link
Contributor Author

yarko commented Jan 31, 2014

@jbau would you look at this also?

@jbau
Copy link

jbau commented Jan 31, 2014

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

@yarko
Copy link
Contributor Author

yarko commented Jan 31, 2014

@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
Copy link

jbau commented Feb 2, 2014

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
Copy link
Contributor Author

yarko commented Feb 2, 2014

@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
Copy link

jbau commented Feb 2, 2014

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
Copy link
Contributor Author

yarko commented Feb 3, 2014

@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
Copy link
Contributor Author

yarko commented Feb 5, 2014

@yarko
Copy link
Contributor Author

yarko commented Feb 5, 2014

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
Copy link
Contributor Author

yarko commented Feb 5, 2014

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
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
Copy link
Contributor Author

yarko commented Mar 8, 2014

@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
Copy link
Contributor Author

yarko commented Mar 11, 2014

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

@singingwolfboy
Copy link
Contributor

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

@yarko
Copy link
Contributor Author

yarko commented Mar 27, 2014

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