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

Prepare for Connectbox interface #28

Merged
merged 4 commits into from
Dec 13, 2016
Merged

Conversation

edwinsteele
Copy link
Collaborator

@furnox is going to integrate the client side connect box interface (that was developed at the Boise hack). This prepares for that by:

  • Removing the old biblebox interface
  • Restructuring the web root to simplify the nginx config
  • Exposing the contents of the USB drive as UTF-8 encoded json (per the nginx-autoindex module)
  • Adding some sample content so we can see UTF-8 filenames, including arabic so we have a RtL example

- Expose directories of usb drive as auto-indexed json (ready for
  connectbox)
- Add optional sample content role in the format expected by connectbox
Create placeholder index while we import the connectbox interface
- name: update permissions on biblebox config files
file:
dest: "{{ biblebox_config_root }}"
- name: Create placeholder index file
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll eventually need something here to copy the client interface to biblebox_default_content_root, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's correct. You can remove the task that creates the placeholder index file, and replace it with a task like we have for copying/synchronising the admin ui. PM on slack if you're unfamiliar with ansible - happy to talk you through it.

Copy link
Contributor

@furnox furnox left a comment

Choose a reason for hiding this comment

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

Looks good to me. I tested the nginx config with what I had and it seemed to work. I'm assuming that the client interface should go under html.

@edwinsteele
Copy link
Collaborator Author

Thanks, and yes, the client interface should go under html.

# pass the PHP scripts to FastCGI server listening on 127.0.0.1:9000
# Admin interface
location /admin/ {
root {{ biblebox_web_root }};
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be biblebox_admin_root?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only if we had alias {{ biblebox_admin_root }}; instead. Given /admin is a common suffix, I'm told it's more idiomatic to use a root element, even though they're functionally identical.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, shouldn't it be
root {{ biblebox_admin_root }};
instead of
root {{ biblebox_web_root }};

biblebox_admin_root is where the files are being synchronized to in main.yml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When you use root, the path on disk is the value of the root directive and the URL. The url starts with /admin/ in this case, so if we used {{ biblebox_admin_root }}, nginx would be looking for files under /var/www/biblebox/admin/admin. The alias directive doesn't append the URL when nginx is looking for files on disk so alias {{ biblebox_admin_root }} is equivalent to what we have. Perhaps you're thinking of alias?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I'm just not real familiar with nginx configs. :-)
I didn't realize that root appended the url. This makes perfect sense now.

Copy link
Collaborator

@kldavis4 kldavis4 left a comment

Choose a reason for hiding this comment

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

Tests looks good. It looks like the web server changes are transitional. My only question is about whether going forward this will have the ability to ship content on the device image and override it on the usb.

@@ -46,27 +55,14 @@ nginx_vhosts:
auth_basic_user_file /usr/local/biblebox/etc/basicauth;
}

location /shared/ {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The way I am understanding this, it looks like the fallback to web root content is no longer happening with this change. The intention of that was to allow the device to be shipped with some on device content not on the usb stick that would be merged with whatever is already on the device.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the intent of the biblebox itself was that all content could be quickly removed and hidden/destroyed? I'm a little late to the party so I'm sure I missed some discussion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The fallback to web root content has been taken away, but I took it out without realising the intent :-\ I think it's nice to have that feature, so let's merge this (so @furnox can continue) and I'll put it back in a followup PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure about the desire to quickly remove/hide content. I haven't heard about that use-case, but it'd be good to ask

@edwinsteele
Copy link
Collaborator Author

I've created #29 to track the work to re-instate the ability to display non-USB content alongside USB content.

@edwinsteele edwinsteele merged commit 9c3ac68 into master Dec 13, 2016
@edwinsteele edwinsteele deleted the integrate-connectbox-interface branch December 13, 2016 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants