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

Apps separate from sdk 1154 #1231

Merged
merged 17 commits into from Mar 16, 2015
Merged

Apps separate from sdk 1154 #1231

merged 17 commits into from Mar 16, 2015

Conversation

ioanalianabalas
Copy link
Contributor

For #1154.

@andy-berry-dev
Copy link
Member

@ioanalianabalas This looks good. It might be worth adding 1 more test to use an entirely different temporary directory rather just the parent file of where BRJS is. You can use the FileUtils class to create a temporary directory and use that for the working dir (remember to delete the new temporary dir in your @After teardown). This can go into ReadyForTest when that's done.

@ioanalianabalas
Copy link
Contributor Author

@andyberry88 I added the test.

@dchambers
Copy link
Contributor

@ioanalianabalas, there are merge conflicts you now need to resolve.

@dchambers
Copy link
Contributor

@andyberry88, are you happy with the extra test @ioanalianabalas added based on your feedback?

@dchambers
Copy link
Contributor

I've dev-reviewed the changes that came after that test and am happy with them.

@andy-berry-dev
Copy link
Member

Yep. Moved to ReadyForTest. Although I'm afraid @ioanalianabalas has some more conflicts to fix.

@ioanalianabalas
Copy link
Contributor Author

@andyberry88 I have fixed the merge conflicts.

@andy-berry-dev andy-berry-dev added this to the 1.0 milestone Mar 4, 2015
@andy-berry-dev andy-berry-dev modified the milestones: 1.0 RC1, 1.0 Mar 4, 2015
@thanhc
Copy link
Contributor

thanhc commented Mar 9, 2015

seems to have a mixup with what should happen when we have both apss and brjs-apps folders, the warning message and site docs say we use brjs-apps but implementation and also for backwards comp we should use apps

@andy-berry-dev
Copy link
Member

Both the logging and site docs have been updated

@andy-berry-dev andy-berry-dev removed their assignment Mar 10, 2015
@thanhc
Copy link
Contributor

thanhc commented Mar 10, 2015

creating an app via the dashboard for the first time is throwing the following error:

Successfully deployed 'ggrtrtg1k' app

Exception in thread "Thread-11" java.lang.NullPointerException
        at org.bladerunnerjs.appserver.AppDeploymentFileWatcher.deployApp(AppDeploymentFileWatcher.java:108)
        at org.bladerunnerjs.appserver.AppDeploymentFileWatcher.checkForNewApps(AppDeploymentFileWatcher.java:83)
        at org.bladerunnerjs.appserver.AppDeploymentFileWatcher.run(AppDeploymentFileWatcher.java:57)

after this creating other apps does not throw the error, but if you restart brjs then the error will be displayed when creating a new app, only seems to happen when brjs is running, using the brjs create-app command does not throw the error.

@thanhc
Copy link
Contributor

thanhc commented Mar 10, 2015

Also should we prevent people from creating an app called brjs-apps, currently it is possible to create an app with this name and so if you are in this folder and then run create-app it will place the new app in this folder.

@andy-berry-dev
Copy link
Member

creating an app via the dashboard for the first time is throwing the following error:

We've probably hardcoded the apps dir in the AppDeploymentFileWatcher. This should use the BRJS instance to get the apps dir.

Also should we prevent people from creating an app called brjs-apps

IMO this is a real edge case and you deserve what you get if you do it. Do we also stop people creating folders called brjs-apps within an app too?

thanhc added a commit that referenced this pull request Mar 16, 2015
@thanhc thanhc merged commit 9345e74 into develop Mar 16, 2015
@thanhc thanhc deleted the apps-separate-from-sdk-1154 branch March 16, 2015 08:56
@thanhc
Copy link
Contributor

thanhc commented Mar 16, 2015

retested issue and the error no longer occurs when creating app from dashboard when there is no apps folder

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

4 participants