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

Added brainstorm to list of apps #98

Merged
merged 1 commit into from
Aug 10, 2015
Merged

Added brainstorm to list of apps #98

merged 1 commit into from
Aug 10, 2015

Conversation

Azeirah
Copy link
Contributor

@Azeirah Azeirah commented Aug 9, 2015

As requested by @paulproteus I've added brainstorm to the list of sandstorm apps.

@paulproteus
Copy link
Collaborator

Thanks @Azeirah !

I took the time to try the package out, and I have some feedback, in case you're interested. @kentonv is the one who actually merges these pull requests onto the website; I don't know if they're showstoppers for him or not.

So I'll file a few bugs against the original app, and provide links here. I'm grateful that you're uploading this, and I look forward to other Sandstorm users using this app!

@paulproteus
Copy link
Collaborator

OK I think the only issues I could come up with were:

Thanks again to you for choosing to share the package more widely! I hope it's OK for me to provide a quick review like this and file issues; these comments are distributed in the hope that they will be useful.

Also, it'd be lovely if you could distribute the sandstorm-files.list and sandstorm-pkgdef.capnp as part of the brainstorm repo. That way, if someone else wants to modify these for their own use, or submit pull requests against them, it would be possible.

@paulproteus
Copy link
Collaborator

Assigning this to @kentonv for merging into the app list.

UPDATE: I can't do that because I don't have the power to assign on this repo! Never mind.

@Azeirah
Copy link
Contributor Author

Azeirah commented Aug 9, 2015

Thanks for opening the issues @paulproteus. Where can I find this sandstorm-files.list file? It's not in my local brainstorm directory.

@paulproteus
Copy link
Collaborator

It depends how you did the packaging. Are you using vagrant-spk per https://docs.sandstorm.io/en/latest/vagrant-spk/packaging-tutorial/ , or the raw spk tool per https://docs.sandstorm.io/en/latest/developing/raw-packaging-guide/ ? vagrant-spk is new since ~March, so probably you used the raw tooling, now that I think about it.

Actually either way I think it should be in the same directory as sandstorm-pkgdef.capnp. If you can't find it there, then hmm, you might have lost it; it's not a huge deal.

@Azeirah
Copy link
Contributor Author

Azeirah commented Aug 9, 2015

@paulproteus I was using meteor-spk. At no point was that file generated, I ran sudo meteor-spk pack.

@paulproteus
Copy link
Collaborator

Ah hah, right, I forgot about meteor-spk!

@ocdtrekkie
Copy link
Collaborator

Probably should document on the repo somewhere that it's built with meteor-spk then.

@paulproteus
Copy link
Collaborator

Neither of these are data-loss bugs, and neither of them are usability show-stoppers, IMHO.

@kentonv my vote is to merge this pull request and let any further changes come in as follow-up pull requests.

kentonv added a commit that referenced this pull request Aug 10, 2015
Added brainstorm to list of apps
@kentonv kentonv merged commit 1acc3b2 into sandstorm-io:master Aug 10, 2015
@kentonv
Copy link
Member

kentonv commented Aug 10, 2015

Hey @Azeirah, I see you updated brainstorm.spk to fix one of Asheesh's concerns. Thanks! Note that in the future, when you upload new versions, it will be very important that you put the new version at a new URL while keeping the old version available at the original URL, because Sandstorm checks the package against a hash before installing and will reject the package if its hash does not match. You'll need to send a pull request here to update the app list to the new version.

Thanks!

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.

4 participants