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

Allow dynamic skin files to be stored in S3. (updated for uPortal 5) #735

Merged

Conversation

jhelmer-unicon
Copy link
Contributor

https://issues.jasig.org/browse/UP-4677

This PR is a revamp of a previous PR that was created before the 5.x branch was created.

groybal and others added 6 commits September 13, 2016 09:42
…ybal-UP-4677

Conflicts:
	uportal-war/src/main/resources/properties/contexts/applicationContext.xml
…ybal-UP-4677

Also, clean up scan to look for specific beans vs. using a regexp.

Conflicts:
	uportal-war/src/main/resources/properties/contexts/applicationContext.xml
… options into portal.properties vs. introducing a new set of property files.
# Conflicts:
#	uportal-war/src/main/java/org/jasig/portal/portlets/dynamicskin/DynamicRespondrSkinConfigController.java
#	uportal-war/src/main/java/org/jasig/portal/portlets/dynamicskin/DynamicRespondrSkinViewController.java
#	uportal-war/src/main/resources/properties/contexts/applicationContext.xml
#	uportal-war/src/main/resources/properties/contexts/flowsContext.xml
#	uportal-war/src/main/resources/properties/contexts/portlet/DynamicRespondrSkin-portlet.xml
#	uportal-war/src/main/resources/properties/contexts/portlet/GoogleAnalytics-portlet.xml
#	uportal-war/src/main/resources/properties/contexts/portlet/Translator-portlet.xml
#	uportal-war/src/main/resources/properties/contexts/servlet/mvcServletContext.xml
#	uportal-war/src/main/resources/properties/portal.properties
@jhelmer-unicon jhelmer-unicon changed the title UP-4677 uportal5 Allow dynamic skin files to be stored in S3. (updated for uPortal 5) Oct 19, 2016
# Conflicts:
#	uportal-war/src/main/java/org/jasig/portal/portlets/dynamicskin/DynamicRespondrSkinConfigController.java
#	uportal-war/src/main/java/org/jasig/portal/portlets/dynamicskin/DynamicRespondrSkinViewController.java
#	uportal-war/src/main/resources/properties/contexts/applicationContext.xml
#	uportal-war/src/main/resources/properties/contexts/flowsContext.xml
#	uportal-war/src/main/resources/properties/contexts/portlet/DynamicRespondrSkin-portlet.xml
#	uportal-war/src/main/resources/properties/contexts/portlet/GoogleAnalytics-portlet.xml
#	uportal-war/src/main/resources/properties/contexts/portlet/Translator-portlet.xml
#	uportal-war/src/main/resources/properties/contexts/servlet/mvcServletContext.xml
#	uportal-war/src/main/resources/properties/portal.properties
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 22.979% when pulling 232ffc3 on jhelmer-unicon:UP-4676_uportal5 into 7dd2a50 on Jasig:master.

Copy link
Member

@ChristianMurphy ChristianMurphy 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

@bjagg
Copy link
Member

bjagg commented Dec 30, 2016

@jhelmer-unicon We need to coordinate on this to get the changes merged before others create conflicts.

@ChristianMurphy
Copy link
Member

@jhelmer-unicon merge conflicts have cropped up, could you resolve them?

…uPortal into jhelmer-unicon-UP-4676_uportal5

# Conflicts:
#	uportal-war/src/main/java/org/apereo/portal/portlets/dynamicskin/FileSystemDynamicSkinService.java
#	uportal-war/src/main/resources/properties/contexts/applicationContext.xml
# Conflicts:
#	uportal-war/src/main/java/org/apereo/portal/portlets/dynamicskin/FileSystemDynamicSkinService.java
#	uportal-war/src/main/resources/properties/contexts/applicationContext.xml
Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @jhelmer-unicon!

@jhelmer-unicon
Copy link
Contributor Author

Merge conflicts fixed. I completely forgot this PR was still outstanding. Baring down-votes, I'll probably try to get this merged in the next few days.

@ChristianMurphy
Copy link
Member

@jhelmer-unicon Travis CI tests are timing out, during cobertura.
There is an exception happening https://travis-ci.org/Jasig/uPortal/builds/202711024#L5986
But that same exception appears to be happening on master https://travis-ci.org/Jasig/uPortal/builds/202722019#L6205

Copy link
Member

@apetro apetro left a comment

Choose a reason for hiding this comment

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

Skimmed. Little skin in the game, as MyUW not using dynamic skins.

Observation: this feels like a lot of code and complexity for a fringe feature. It'll be a really good day when this kind of thing can be implemented in a separate repository that compiles against semantically versioned uPortal APIs and builds a separate .jar that becomes an optional dependency published to Maven Central that one pulls into a uPortal implementation iff one needs this feature.

I appreciate we're not there yet. Progress is progress. 👍. That'll be more progress.

@ChristianMurphy
Copy link
Member

@jhelmer-unicon any update on this?

@ChristianMurphy
Copy link
Member

@jhelmer-unicon a merge conflict has cropped up, could you resolve it?

# Conflicts:
#	pom.xml
#	uportal-war/src/main/java/org/apereo/portal/portlets/dynamicskin/DynamicRespondrSkinConfigController.java
#	uportal-war/src/main/java/org/apereo/portal/portlets/dynamicskin/DynamicRespondrSkinViewController.java
#	uportal-war/src/main/java/org/apereo/portal/portlets/dynamicskin/DynamicSkinService.java
#	uportal-war/src/main/java/org/apereo/portal/portlets/dynamicskin/FileSystemDynamicSkinService.java
@ChristianMurphy ChristianMurphy added this to the 5.0.0 milestone Apr 1, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 23.768% when pulling 7923be1 on jhelmer-unicon:UP-4676_uportal5 into 57f3681 on Jasig:master.

@jhelmer-unicon jhelmer-unicon merged commit 87014c6 into uPortal-Project:master Apr 4, 2017
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.

None yet

6 participants