Skip to content

Fix paver watch_assets - #11849

Merged
omarkhan merged 1 commit into
openedx:masterfrom
open-craft:omar/watch-assets
Apr 4, 2016
Merged

Fix paver watch_assets#11849
omarkhan merged 1 commit into
openedx:masterfrom
open-craft:omar/watch-assets

Conversation

@omarkhan

Copy link
Copy Markdown
Contributor

Background: paver watch_assets does not work in devstack. This pull request fixes it with the following changes:

  • Update to latest version of watchdog.
  • inotify doesn't work on nfs share in devstack. Use polling instead.
  • Some editors (vim) do not trigger modify events in their default configuration. Rebuild assets on all filesystem changes, and debounce when changes happen too quickly.

JIRA tickets: OPS-169, FEDX-109

Testing instructions:

  • Run paver watch_assets in devstack
  • Modify a sass file

Previous behaviour: nothing happens

Starting asset watcher...

After fix:

Starting asset watcher...
  CHANGED: cms/static/sass/_base.scss
---> pavelib.assets.compile_sass
    Finished compiling Sass:
>> common/static/sass -> common/static/css in 0:00:00.023160s
>> lms/static/sass -> lms/static/css in 0:00:19.277999s
>> lms/static/themed_sass -> lms/static/css in 0:00:00.002187s
>> lms/static/certificates/sass -> lms/static/certificates/css in 0:00:32.928536s
>> cms/static/sass -> cms/static/css in 0:00:17.807262s

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @omarkhan! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?number=11849&repo=edx%2Fedx-platform

@omarkhan
omarkhan force-pushed the omar/watch-assets branch from 9967605 to ff9b612 Compare March 18, 2016 07:24
@omarkhan

Copy link
Copy Markdown
Contributor Author

@Kelketek ping in case you missed this on the JIRA ticket

@Kelketek

Copy link
Copy Markdown
Contributor

👍

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @omarkhan! I've created OSPR-1201 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:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

Comment thread pavelib/assets.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems like this could lead to an unfortunate circumstance of:

Time 0.0 -- Change A -> Triggers rebuild
Time 0.1 -- Change B -> Doesn't trigger

At that point, Change B won't be seen until some other change C happens.

Would it be possible to instead have a timer that gets kicked off/restarted by any change, which would wait 1 second, and after a second drop an item on the queue?

That would lead to the behavior:

Time 0.0 -- Change A -> Triggers rebuild at 1.0
Time 0.1 -- Change B -> Cancels rebuild at 1.0, triggers rebuild at 1.1.

Said another way: rather than dropping all within wait_seconds of a build starting, wait for wait_seconds after the most recent change to start a build.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. I will make this change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@cpennington done in 8e50fac

@bjacobel

Copy link
Copy Markdown
Contributor

LGTM aside from pylint issues.

@omarkhan

Copy link
Copy Markdown
Contributor Author

@bjacobel pylint issues fixed in 810658c

@bjacobel

Copy link
Copy Markdown
Contributor

👍

@omarkhan
omarkhan force-pushed the omar/watch-assets branch from 810658c to f286de1 Compare March 28, 2016 18:57
@omarkhan

Copy link
Copy Markdown
Contributor Author

Squashed commits

@omarkhan

Copy link
Copy Markdown
Contributor Author

jenkins run bokchoy

@jbarciauskas

Copy link
Copy Markdown
Contributor

@omarkhan looks like there are conflicts?

@omarkhan

Copy link
Copy Markdown
Contributor Author

@jbarciauskas rebased

- Update to latest version of watchdog.
- inotify doesn't work on nfs share in devstack. Use polling instead.
- Some editors (vim) do not trigger modify events in their default
  configuration. Rebuild assets on all filesystem changes, and debounce
  when changes happen too quickly.
@omarkhan
omarkhan force-pushed the omar/watch-assets branch from 07d74ba to 5364257 Compare April 4, 2016 05:56
@bjacobel

bjacobel commented Apr 4, 2016

Copy link
Copy Markdown
Contributor

jenkins run js

@bjacobel

bjacobel commented Apr 4, 2016

Copy link
Copy Markdown
Contributor

@omarkhan Looks ready to merge.

@omarkhan
omarkhan merged commit 66ac9b0 into openedx:master Apr 4, 2016
@omarkhan
omarkhan deleted the omar/watch-assets branch April 4, 2016 15:37
@omarkhan

omarkhan commented Apr 4, 2016

Copy link
Copy Markdown
Contributor Author

Thanks @bjacobel!

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants