WIP: Use grunt for assets, remove django pipeline, remove ruby, use bower for client-side deps… - #6026
WIP: Use grunt for assets, remove django pipeline, remove ruby, use bower for client-side deps…#6026tusbar wants to merge 38 commits into
Conversation
|
Thanks for the pull request, @tusbar! I've created OSPR-220 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Feel free to add as much of the following information to the ticket:
All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here. |
|
😍 |
|
@benpatterson did you have a suggestion on how they can install bower? |
|
@tusbar I see the bower install command in package.json, but I didn't notice actually installing bower itself in the package.json changes. |
|
@jzoldak currently installing locally, in |
|
gotcha. thanks @tusbar. I see you referenced the path in your last commit; that makes sense. For making permanent changes to the scripts & install files (I know this is a WIP), we'd want to think about using something like nodeenv for our CI pipeline. |
|
@benpatterson why not install them globally? we’re talking CI, what’s the point of having an isolated environment within that disposable environment? |
|
@tusbar good question. In our current CI infrastructure, the workers run multiple jobs before they are shut down. Beyond that is the context of a dev environment (e.g., devstack); we'd want some ways of assuring cleanliness from branch to branch, if possible. |
3bee7da to
73e40fb
Compare
|
@talbs I’ve found the issue with neat 1.7.0. (the unitless error) It’s here: https://github.com/tusbar/edx-platform/blob/hackathon/use-grunt-for-assets/lms/static/sass/views/_verification.scss#L1391 Though Also, this line is weird: https://github.com/tusbar/edx-platform/blob/hackathon/use-grunt-for-assets/lms/static/sass/views/_verification.scss#L1152 |
|
I’d also like to get input on this: what do you think about merging all these files into one file:
And same thing for They’re always included together, so why have 3 (6) files included in the HTML and not just 1 (2)? They could share the same SASS file as well ( Some numbers:
|
Still using ruby sass for now, waiting for lbsass.
Add LMS task to run both 'sass' and 'concat' tasks. Add time-grunt to display how long tasks take to run.
Bourbon’s margin mixin was overriding bi-app-ltr’s
Add studio watch task. Create base, :dev and :dist tasks for each environment.
|
@tusbar there's some stuff you need to respond to in the above comments so the status is "waiting on author". Please address the comments then ping @singingwolfboy when you'd like attention again to this work. |
|
Nice work @tusbar and @felipemontoya. I second @talbs thoughts. I think it'd also be nice to work with as few "things" as possible. So, if adding BlessCSS reduces a step, then I give it two thumbs up. |
|
@tusbar we are really excited about the prospect of this change. However looks like you haven't been responsive on this pull request - that's totally OK but I'm going to close this pull request for now, until you are ready to work on it again. It is a lot of maintenance work for us to keep it open and keep checking on its progress. Anyway - once you are ready to respond to the comments and rebase this PR, please feel free to reopen it. |
|
Does commenting on it reopen it? |
|
No it doesn’t, @sarina, could you do so, please? I actually work on this regularly, though I have nothing to answer yet, since I don’t have anything to show YET. I cannot allow 100% of my time to this, even though I would like to. I don’t think that closing the PR is the way to go, it’ll just get forgotten. And I believe that this is counterproductive to community management. |
|
@tusbar just continue hacking :-) |
|
@tusbar it's more likely to be forgotten if it's just sitting around open. I think it's a better strategy to continue hacking on your fork, and when it's ready for more code review, we can reopen. When a PR has momentum (currently being reviewed and quickly responded to), that is the best case scenario for it. |
|
@sarina I disagree. The branch has changed since the PR was closed, and that does not update the PR, nor run the test build (which the last commit aims to fix). Finally, I don’t exactly feel “free to reopen it”, as I am not allowed to do so. |
|
Hey, all. Very selfishly (as this good work will safe part of my designer/front end developer soul), I'm happy to help keep this ball in the air. @sarina, if you're okay with keeping this open, I can actively ping folks to review and try to get formal edX support. To some of sarina's earlier points, keeping it open also means we need to keep on progress on your end @tusbar. That means addressing feedback once it comes as well as any other loose ends. Can you commit to a certain # of hours/effort a week to this? Let me know if any of that is agreeable and we can keep this good stuff on the road to Mergetown. Thanks again to both of you for trying to help! |
|
Hi, I talked briefly to @tusbar yesterday and I will be writing a guide on how to set up and test locally. |
|
In our experience, pull requests in the edx-platform repo that aren't being reviewed and don't have new commits tend to fade away and never get merged. So, I just want open pull requests to have a level of activity that ensures they won't be forgotten. For some reason I can't reopen this pull request. @singingwolfboy - git wizard - any advice? |
|
@sarina nope, I can't reopen it either. The button is grayed-out and disabled, with a tooltip message "The hackathon/use-grunt-for-assets branch was force-pushed or recreated." I'd suggest opening a new pull request from the same branch, and we just abandon this pull request. Github doesn't seem to like force-pushing branches with pull requests, which is strange, since we've never had a problem with it before. |
|
FYI @tusbar - this is weird github behavior and I'm reaching out to their support team for elaboration. |
|
Regardless of how this is reopened, I just wanted to react to @felipemontoya 's point about writing a guide on how to build/test locally. I'd expect we could use the same processes (e.g., devstack, build system) that are used for all pull requests. Unless there's something I'm missing? For Jenkins, I'd expect this change would work out how we'd use npm packages in these systems if they're not readily available on the PATH. I recall some changes to that effect in scripts/all-tests.sh that were being considered, for example. |
|
@singingwolfboy force pushes work fine with open PRs. I know that github freezes closed PRs, so that might be why it's having issues w/ the force-push. |
|
@cpennington do you know why github freezes closed PRs? I've been operating On Thu, Dec 18, 2014 at 9:59 AM, Calen Pennington notifications@github.com
|
|
@sarina It's not that it blocks you from pushing to the branch, or anything. And usually you can reopen. I'm not sure why it got so confused in this case. But I think they freeze them in part so that they don't have to keep updating the diff against master, and the like (certainly, if I were running github, that's a reduction in backend workload that I'd want to make). |
|
@cpennington ah, that makes sense. Yeah, I've never seen this. I had a PR last week that I had pushed new commits to (but the diff hadn't updated); I reopened the PR and the diff updated appropriately. @tusbar I haven't heard back from Github. You can wait to see if they get back to me with a resolution, or you can open a new PR and reference this one. Sorry about that. I won't close PRs again without ample notice, I suppose. |
|
@sarina no problem. I can open a new one. I’m all for the notice, at least some kind of "this will be closed if no answer in X days". |
|
@tusbar awesome, thanks. Github got back to me with basically, "yeah rebasing has bugs, too bad for you" 😢 😞 |
|
@benpatterson It is mostly the same process. There are some extra goodies that I wanted to point in the guide. In any case, I wrote it in the wiki now. I will update it when and if the pull request changes. @tusbar could you please look at it and let me know if I forgot something, or if something is wrong 😁. |
|
@tusbar and @felipemontoya, thanks again for the continued progress with this. Just a heads up, I found the following Neat upgrade in my feeds today - https://twitter.com/bourbonsass/status/551022374179856384 ("...with better Libsass compatibility"). Might be worth upgrading this branch to see if this helps with the versioning clashing/compromising you folks had to do during the Hackathon. |
|
@explorerleslie: this is the PR that @marcotuts and I were talking about re: asset management improvements. Not sure if there's a newer version of this... @talbs, @sarina? |
|
Thanks for the ping, @ormsbee. I'd love to help prioritize this work given that it's expected to have significant impact on LMS performance, which is a focus for TNL right now. A couple direct questions: @tusbar and @felipemontoya, are you going to open a new PR for this work? From the comments above, it seems like that was what was decided, but I haven't seen a new one come in. After that, what is left before you're in a place where edX can review this PR again? Do you have an estimated time until we can have another look? |
|
@explorerleslie: @tusbar is going to be busy for the next week, so I will take the time later today to open the new PR and do a rebase to get this going again. |
|
@felipemontoya I'm an interested party here as well...whenever that gets done, could I be CC'd? Psyched to hear this could get moving again. |
|
@explorerleslie - yes, unfortunately I didn't realize that a closed pr can't be reopened. I won't do it again, but this one needs to have a new pull request opened. |
|
@felipemontoya - great, that sounds good. @benpatterson i will be sure to loop you in. |
15 participants
This PR was created at the Open edX Con hackathon.
Here’s a list of what it does:
Cool things:
TODO: