Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

Mirador #42

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

flummingbird
Copy link

@flummingbird flummingbird commented Jun 1, 2017

GitHub Issue: (Islandora/documentation#645)

What does this Pull Request do?

downloads the drupal 8 mirador module, which is note based off of core mirador but rather an obscure drupal friendly branch. Also pulls the mirador javascript dependency.

Feedback todos from last pull request have been completed.
Downloads and installs mirador drupal8 module, and JavaScript dependencies.

What's new?

permission issues fixed thanks to @ruebot @whikloj and @dannylamb

  • Added scripts/mirador.sh

  • Could this change impact execution of existing code?
    no

How should this be tested?

  • To Test that the Pull Request does what is intended:

clone the repository

cd into repository folder

vagrant up

check localhost:8000/admin/config/media/mirador

Additional Notes:

I branched my own fork, I'm not sure if I can create a branch on the offical claw_vagrant to merge my PR into. Suggestions?

Interested parties

@Islandora-CLAW/committers @whikloj @ruebot @dannylamb

…kips a few steps of mirador install for you.
added needed line to Vagrant file for /scripts/mirador.sh currently commented out bc I haven't tested a clean build of all this together.

rm /tmp/mirador-8.x-1.x-dev.zip

#echo 'drush en mirador -y'
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove

@whikloj
Copy link
Contributor

whikloj commented Jun 2, 2017

Ahhh I didn't realize there was a new PR. This supercedes #41.

@whikloj
Copy link
Contributor

whikloj commented Jun 2, 2017

@flummingbird so the only thing I would suggest is to perhaps take a look at this section of another script.

We download into a specific location and only if the file doesn't exist.

That way if you do a vagrant destroy; vagrant up and the versions haven't changed you don't need to re-download those files. We just copy them into the /tmp directory (for example) and move ahead.

Reduces time on rebuilding and allows you to build a machine while offline.

@ruebot
Copy link
Contributor

ruebot commented Jun 5, 2017

@flummingbird once you take care of @whikloj's request, I'll test. Let me know if you need a hand with what @whikloj is talking about.

conditional download and enabling, prevents repeated downloads, and uses proper $DOWNLOAD_DIR variable. I have tested it locally and it works like a charm.
Copy link
Author

@flummingbird flummingbird left a comment

Choose a reason for hiding this comment

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

I've made some changes, I believe it is ready.

uncouple download to wait for each check. Enable and unzip go outside the conditional downloads.
puts unzip back in conditional check.
Vagrantfile Outdated
@@ -57,6 +57,7 @@ Vagrant.configure(VAGRANTFILE_API_VERSION) do |config|
config.vm.provision :shell, :path => "./scripts/islandora-karaf-components.sh", :args => home_dir
config.vm.provision :shell, :path => "./scripts/config.sh", :args => home_dir
config.vm.provision :shell, :path => "./scripts/crayfish.sh", :args => home_dir
config.vm.provision :shell, :path => "./scripts/mirador.sh", :args => home_dir
Copy link
Contributor

Choose a reason for hiding this comment

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

if [ ! -f "$DOWNLOAD_DIR/master.zip" ]; then
echo "Downloading mirador-js dependencies"
cd "$DOWNLOAD_DIR" && { curl -LOk https://github.com/NVLI/mirador-js/archive/master.zip; cd -;}
cd "$DRUPAL_HOME"/web/libraries && { unzip "$DOWNLOAD_DIR"/master.zip -d . ;}
Copy link
Contributor

Choose a reason for hiding this comment

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

@flummingbird this line needs to move outside the if...fi (line 16) as it should occur whenever a box is provisioned and is separate from the downloading.
The way I think of it is that lines 10-12, is just checking if you have downloading the application before.
Then line 13 is deploying that application into your vagrant machine.

So you can conceivably run lines 10-12 once and use line 13 waaaaaay more than that.

if [ ! -f "$DOWNLOAD_DIR/mirador-8.x-1.x-dev.zip" ]; then
echo "Downolading mirador drupal module"
cd "$DOWNLOAD_DIR" && { curl -LOk https://ftp.drupal.org/files/projects/mirador-8.x-1.x-dev.zip ;}
cd "$DRUPAL_HOME"/web/modules/contrib/ && { unzip "$DOWNLOAD_DIR"/mirador-8.x-1.x-dev.zip -d . ;}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing as described above

fi

if [ ! -f "$DOWNLOAD_DIR/mirador-8.x-1.x-dev.zip" ]; then
echo "Downolading mirador drupal module"
Copy link
Contributor

Choose a reason for hiding this comment

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

typo Downolading

Copy link
Author

Choose a reason for hiding this comment

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

phooey

typey type-OH
unzips outside of DL function.
@ruebot
Copy link
Contributor

ruebot commented Jun 5, 2017

vagrant up

@ruebot
Copy link
Contributor

ruebot commented Jun 5, 2017

screenshot from 2017-06-05 15-56-45

Looks good.

Copy link
Contributor

@ruebot ruebot left a comment

Choose a reason for hiding this comment

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

@whikloj I'll leave you to merge this once you're good with it.

@flummingbird
Copy link
Author

🕺

@whikloj
Copy link
Contributor

whikloj commented Jun 23, 2017

@flummingbird just wanted to let you know, I haven't forgotten about this. I think due to the Mirador module only working against the image type and image not allowing you to upload anything that isn't a gif, jpeg, or png... we need to hold this until 8.4.x is released and we move to it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants