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.

Sign up for GitHub

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

Jump to bottom

Fix #2778 frontend.url, frontend.url.runtime props #2974

Merged
merged 1 commit into from
Aug 17, 2017
Merged

Fix #2778 frontend.url, frontend.url.runtime props #2974

merged 1 commit into from
Aug 17, 2017

Conversation

inodb
Copy link
Member

@inodb inodb commented Aug 11, 2017 •
edited
Loading

  • frontend.url allows us to specify what cbioportal-frontend url should be used. It overrides whatever frontend version is provided in the WAR through jitpack.
  • frontend.url.runtime allows us to specify a file that contains the runtime version (eventually this should also support a URL)

Also allow changing FRONTEND_URL env variable on heroku

Fix #2778

@inodb inodb requested review from alisman and jjgao August 11, 2017 23:28
@inodb
Copy link
Member Author

inodb commented Aug 11, 2017

@alisman One advantage of this is that we can for instance point to cbioportal-frontend.herokuapp.com and always have latest master branch.

I've got an idea to deal with cache busting as well. We can just add the commit hash in the dist/ folder of the frontend repo. Then we can use that as the cache buster parameter in the script include tag for the frontend. Either do AJAX call right before including or if it's slow we can use localStorage and store it there, then offer user unobtrusive option for reload.

@jjgao jjgao had a problem deploying to cbioportal-pr-2974 August 14, 2017 21:33 Failure
@inodb inodb force-pushed the fix-2778-frontend-url-prop branch from 948dc56 to 9823dd0 Compare August 14, 2017 21:39
@jjgao jjgao temporarily deployed to cbioportal-pr-2974 August 14, 2017 21:40 Inactive
@inodb inodb mentioned this pull request Aug 14, 2017
Merged
@inodb
Copy link
Member Author

inodb commented Aug 14, 2017 •
edited
Loading

You can btw see this working here: https://cbioportal-pr-2974.herokuapp.com/case.do#/patient?caseId=P04&studyId=lgg_ucsf_2014 (this is using a different version than the one specified in the pom, it is using cBioPortal/cbioportal-frontend#562). Changing the frontend on heroku is simply changing the config vars (only restarts the app, no rebuild required)

@jjgao jjgao modified the milestones: 1.8.2, 1.8.3 Aug 16, 2017
@inodb inodb force-pushed the fix-2778-frontend-url-prop branch from 9823dd0 to a661497 Compare August 16, 2017 19:56
@jjgao jjgao temporarily deployed to cbioportal-pr-2974 August 16, 2017 19:56 Inactive
@inodb inodb force-pushed the fix-2778-frontend-url-prop branch from a661497 to f2314bc Compare August 16, 2017 21:33
@jjgao jjgao temporarily deployed to cbioportal-pr-2974 August 16, 2017 21:33 Inactive
@inodb inodb changed the title Fix #2778 Make frontend.url property Fix #2778 Make frontend.url+frontend.url.runtime property Aug 16, 2017
@inodb inodb changed the title Fix #2778 Make frontend.url+frontend.url.runtime property Fix #2778 frontend.url, frontend.url.runtime props Aug 16, 2017
@inodb inodb force-pushed the fix-2778-frontend-url-prop branch from f2314bc to 53bb7d6 Compare August 16, 2017 21:44
@jjgao jjgao temporarily deployed to cbioportal-pr-2974 August 16, 2017 21:44 Inactive
alisman
alisman reviewed Aug 17, 2017
View reviewed changes
portal/src/main/webapp/js/src/load-frontend.js Outdated
} else {
console.log('ERROR: No frontend URL defined, should at least be empty string');
}
// even though this should never happen, probably better to include
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should make it fail loudly though. so devs will notice. by falling back, they could think it's working

Copy link
Member Author

Choose a reason for hiding this comment

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

@alisman yeah that's maybe better. Should never happen anyway. It will just try to include a script tag with undefined plus you'll get the error message above, so that should be p obvious

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

@inodb
Fix cBioPortal#2778 frontend.url+frontend.url.runtime prop
66269bc 
- frontend.url: specify what cbioportal-frontend url should be used. It
  overrides whatever frontend version is provided in the WAR through jitpack.
- frontend.url.runtime: specify a file that contains the runtime version
  (eventually this should also support a URL). Overrides frontend.url.
-  Add FRONTEND_URL env variable to Heroku
@inodb inodb force-pushed the fix-2778-frontend-url-prop branch from 53bb7d6 to 66269bc Compare August 17, 2017 03:01
@alisman alisman merged commit b3171e8 into cBioPortal:hotfix Aug 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alisman alisman alisman approved these changes

@adamabeshouse adamabeshouse adamabeshouse approved these changes

@jjgao jjgao Awaiting requested review from jjgao

@onursumer onursumer Awaiting requested review from onursumer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.8.3
Development

Successfully merging this pull request may close these issues.
4 participants
@inodb @alisman @adamabeshouse @jjgao