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

Initial Support For Deploying To Cloud Foundry #239

Closed
wants to merge 7 commits into from
Closed

Initial Support For Deploying To Cloud Foundry #239

wants to merge 7 commits into from

Conversation

ryanjbaxter
Copy link
Contributor

Cloud Foundry is a popular open source platform-as-a-service. The MEANJS starter project is a great starting point for developers looking to get started with the MEAN stack. This pull request would make it easy to get the MEANJS app deployed to any Cloud Foundry PaaS. This pull request is for the code portion only, there also needs to be some documentation on how someone would deploy the MEANJS app to Cloud Foundry.

@lirantal
Copy link
Member

lirantal commented Nov 3, 2014

Thanks @ryanjbaxter
Instead of pushing everything to the global environment configs, I think it makes more sense to create a specific Cloud Foundry config and place it there so that anyone can use that if required, and we don't "garbage" the project's global vanilla configs.

Also, it would be great if someone can test this before we merge and make sure its tested to work well (I'm not using CF yet personally)

@ryanjbaxter
Copy link
Contributor Author

Thanks for the suggestions @lirantal. I have separated the CF changes into its own config. I also made some changes to support the social login features. While doing this though I had a couple of questions/issues.

  1. I could not get login via LinkedIn to work. My app goes through the OAuth dance but then just ends back at the signin page without actually logging in. I am wondering if others are seeing this problem.
  2. What actions cause emails to be sent?

I also added instructions to the README about how to deploy to Cloud Foundry so others can try this out.

@lirantal
Copy link
Member

lirantal commented Nov 3, 2014

Looks better, thanks.
Let's see if we can get some users to test this and give feedback before we merge.

@NeverOddOrEven
Copy link
Contributor

@ryanjbaxter this is awesome. I just ran through all of this with zero trouble. Your linkedin issue can be described here. I made that prescribed change and LinkedIn login works just fine.

We should advertise all deployment alternatives from the homepage (Heroku, Azure, BlueMix, etc.) as well as getting those steps in the gh-pages branch for the main site.

Otherwise, it looks great.

@ilanbiala
Copy link
Member

I would think a GH wiki or better docs on the MEAN site would be the next step for MEAN to get more popular, because I feel like right now too many issues are being posted that are more questions than bugs or suggestions or things like that in the past couple weeks or so.

@ryanjbaxter
Copy link
Contributor Author

@NeverOddOrEven thanks for pointing out the linkedin issue, sounds like a separate bug.

I am happy to move the documentation out of the REAME and into GH pages. I mostly put it in the README so if anyone wanted to try this out they knew how. I can add the documentation to GH pages in a separate pull request once this pull request is merged.

@@ -0,0 +1,49 @@
var cfenv = require('cfenv'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Add

'use strict';

@NeverOddOrEven
Copy link
Contributor

Noticed a few things tripped up jshint when running locally. Commented on the specific lines.

@ryanjbaxter
Copy link
Contributor Author

Thanks @NeverOddOrEven, should be fixed now.

Not sure why there is a merge conflict with the pull request, my fork should be up to date with this repo and I don't have any merge conflicts....

@ryanjbaxter
Copy link
Contributor Author

Any other comments on this? Can we get this merged?

@ryanjbaxter
Copy link
Contributor Author

Still haven't heard anything, what do you guys think? @lirantal

@rschwabco
Copy link
Member

@ryanjbaxter, I think we should add this to the generator, as one of the optional deployment strategies, as part of the 0.4 branch. Until that happens - we probably shouldn't include all possible deployments master branch at this stage, but it's good to have this PR here - in case anyone wants to use it. wdyt?

@ryanjbaxter
Copy link
Contributor Author

@roieki so is idea to remove heroku and docker support from the master branch and move it to the generator? I have no problem moving it to the generator but there was already support for docker and heroku in the master branch.

@rschwabco
Copy link
Member

Yeah, you're right, we should remove those as well.

@ryanjbaxter
Copy link
Contributor Author

So how do I get started adding this to the generator?

@ilanbiala
Copy link
Member

@ryanjbaxter take a look at https://github.com/meanjs/generator-meanjs structure, and it would probably make sense to add a separate folder for deployments. @NeverOddOrEven may be able to help bring this in.

@NeverOddOrEven
Copy link
Contributor

@ryanjbaxter @ilanbiala I will be addressing the deployment workflows with the generator. Once those are aligned, will likely merge this to 0.4 *docs

@ryanjbaxter
Copy link
Contributor Author

Cool let me know if you need some help!

On Wed, Jan 7, 2015 at 8:13 PM, Alex Suttmiller [email protected]
wrote:

@ryanjbaxter https://github.com/ryanjbaxter @ilanbiala
https://github.com/ilanbiala I will be addressing the deployment
workflows with the generator. Once those are aligned, will likely merge
this to 0.4


Reply to this email directly or view it on GitHub
#239 (comment).

@ilanbiala
Copy link
Member

@NeverOddOrEven should we close this and continue any conversations in the generator repo?

@lirantal
Copy link
Member

@ryanjbaxter this looks like a good PR addition, can you wrap it up for 0.4.0?

@ilanbiala while @roieki voted for moving this to the generator as well as moving out any docker/other deployment method it doesn't seem likely that 0.4.0 generator will be updated anytime soon and it's ok to have it on master/0.4.0 code than nowhere else. WDYT?

@ilanbiala
Copy link
Member

@lirantal Sounds good to me.

@ryanjbaxter
Copy link
Contributor Author

@lirantal let me take a look at this again next week

@lirantal
Copy link
Member

Great, thanks guys.
So @ryanjbaxter I'll be closing this one because it's for master and we need a PR for 0.4.0, once you submit it I'll merge.

@ryanjbaxter
Copy link
Contributor Author

@lirantal see #681 for 0.4.0 pull request

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

Successfully merging this pull request may close these issues.

5 participants