Skip to content

Fixes debug toolbar display issue in CMS on devstack. - #3401

Closed
shindere wants to merge 2 commits into
openedx:masterfrom
shindere:devstack-fix-debug-toobar-display-in-cms
Closed

Fixes debug toolbar display issue in CMS on devstack.#3401
shindere wants to merge 2 commits into
openedx:masterfrom
shindere:devstack-fix-debug-toobar-display-in-cms

Conversation

@shindere

Copy link
Copy Markdown
Contributor

No description provided.

@singingwolfboy

Copy link
Copy Markdown
Contributor

Can you explain the problem that you're fixing, and why this fixes it?

@shindere

Copy link
Copy Markdown
Contributor Author

Problem: when the studio is ran inside of vagrant and one connect sto it
with a browser at http://localhost:8001, the debug toolbar is not shown,
whereas it is shown on the LMS ran in the same conditions when a browser
connects to http://localhost:8000.

The PR fixes the problem because it replaces the default callback which
prevents the debug tool bar from being shown, because the IP from which
one connects is not listed in the internal IPs.

Worth mentionning is that this is how things work in the LMS settings.
So the line added by the PV in the settings of the CMS does also appear
in the LMS settings file.

In other words, this PR makes Studio's devstack.py more similar to
LMS's one, which is working (debug toolbar is displayed correctly).

One other possibility would be to add the IP VirtualBox assigns to the
host machine (10.0.2.2) to the INTERNAL_IPS settings in devstack.py for
both LMS and CMS. Then the line added by this PR would not be necessary,
and the similar line already present in LMS's devstack.py could also be
removed.

I can modify the PR if this second approach is prefered.

@singingwolfboy

Copy link
Copy Markdown
Contributor

Seems reasonable to me. Could you add a comment in the code explaining why you're taking this approach? Also, it looks like this pull request has conflicts with master, so it needs to be rebased. After that, someone on the devops team should take a look at it.

@shindere

Copy link
Copy Markdown
Contributor Author

Rebase done. The conflict was in AUTHORS. And thanks for the rebase wiki
page which is awesome.

Regarding your request to comment the code: given that the patch just
makes the CMS settings similar to the LMS settings,shall the comment
appear in both setting files?

@singingwolfboy

Copy link
Copy Markdown
Contributor

@shindere oh, I didn't realize that you were making cms/envs/devstack.py and lms/envs/devstack.py more consistent with each other -- I just checked, and you're right! More comments are generally better, but in this case I withdraw my request -- I thought that you were making an intentional change to how things work, but you're just trying to make things more consistent, which I don't think needs comments. (But if you want to add them, that would be even better!)

@shindere

Copy link
Copy Markdown
Contributor Author

You mean add comments on both sides? LMS and CMS?

Although I agree that the line I dded in the CMS (and which was already
in the LMS) is really non-trivial and could indeed very well diserve
explanations, I have to say I'm not very comfortable with adding them in
two places because there is the risk that they are then both modified
independently.

How about adding just a link to the PR in both codes?

That would be more concise than re-writing the same explanation in two
places.

@neurolit

Copy link
Copy Markdown
Contributor

Hi! Is someone in devops team taking a look at it?

@e0d

e0d commented May 13, 2014

Copy link
Copy Markdown
Contributor

👍

shindere added 2 commits May 13, 2014 15:18
In Studio, the debug tool bar did not appear as in LMS.
@singingwolfboy

Copy link
Copy Markdown
Contributor

Ugh! The tests passed, but while the tests were running, there was another conflict with master. I've just cherry-picked your changes onto a new branch off of master and merged that branch, so this pull request is effectively merged.

@neurolit

Copy link
Copy Markdown
Contributor

Thanks!

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