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

feat: Upgrade sipi to 2.0.1 #1459

Merged
merged 55 commits into from Oct 18, 2019
Merged

feat: Upgrade sipi to 2.0.1 #1459

merged 55 commits into from Oct 18, 2019

Conversation

subotic
Copy link
Collaborator

@subotic subotic commented Oct 2, 2019

resolves #1437
resolves #1449
resolves #1457
resolves #1472

@subotic subotic self-assigned this Oct 2, 2019
@subotic subotic added the chore maintenance and build tasks label Oct 2, 2019
@subotic subotic added this to the 2019-10 milestone Oct 2, 2019
@subotic subotic changed the title chore: Upgrade sipi to 2.0.1 feat: Upgrade sipi to 2.0.1 Oct 2, 2019
@subotic subotic added enhancement improve existing code or new feature and removed chore maintenance and build tasks labels Oct 2, 2019
Copy link

@benjamingeer benjamingeer left a comment

Choose a reason for hiding this comment

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

The URL of the IIIF images is hardcoded in a JSON file

Where is that file?

Makefile Outdated Show resolved Hide resolved
@subotic
Copy link
Collaborator Author

subotic commented Oct 14, 2019

The URL of the IIIF images is hardcoded in a JSON file

Where is that file?

in ResourcesResponderV1SpecContextData line 43: src/test/resources/test-data/v1/expectedBookContextResponse.json

@benjamingeer
Copy link

in ResourcesResponderV1SpecContextData line 43: src/test/resources/test-data/v1/expectedBookContextResponse.json

OK, I understand now. But that's an important test, we shouldn't disable it. You could turn the JSON file into a template, e.g. replace all 0.0.0.0:1024 with SIPI_HOST. Then in the test, when the file is loaded, replace SIPI_HOST with the configured value.

@subotic
Copy link
Collaborator Author

subotic commented Oct 15, 2019

Would be great :-)

@benjamingeer
Copy link

I mean, just use String.replaceAll(), it will just be one line of code.

@subotic
Copy link
Collaborator Author

subotic commented Oct 17, 2019

I mean, just use String.replaceAll(), it will just be one line of code.

done in e7f47f6. added the test back.

@subotic
Copy link
Collaborator Author

subotic commented Oct 17, 2019

@benjamingeer could you take a quick look?

@benjamingeer
Copy link

Looks OK but something isn't working in the build, and I keep getting emails from Codacy saying that there are issues.

@subotic
Copy link
Collaborator Author

subotic commented Oct 17, 2019

yes, I've renamed some makefile targets and forgot to update .travis.yml. Doing it now.

@subotic
Copy link
Collaborator Author

subotic commented Oct 17, 2019

should be fixed now.

Regarding Codacy, I don't have access to the 'dasch-swiss' organization.

@benjamingeer
Copy link

Regarding Codacy, I don't have access to the 'dasch-swiss' organization.

I don't understand much about Codacy, but for me it displays this PR under dhlab-basel:

Screenshot 2019-10-17 at 14 01 40

@subotic
Copy link
Collaborator Author

subotic commented Oct 17, 2019

ah, makes sense. I will change it.

Copy link

@benjamingeer benjamingeer left a comment

Choose a reason for hiding this comment

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

This looks good to me. If it's done, please merge and many thanks!

@subotic
Copy link
Collaborator Author

subotic commented Oct 18, 2019

your welcome and thanks for the review :-)

@subotic subotic merged commit 3368a3a into develop Oct 18, 2019
@subotic subotic deleted the wip/1457-upgrade-sipi branch October 18, 2019 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improve existing code or new feature
Projects
None yet
3 participants