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

Improve docs (a bit) #288

Merged
merged 2 commits into from Oct 18, 2018
Merged

Improve docs (a bit) #288

merged 2 commits into from Oct 18, 2018

Conversation

chris48s
Copy link
Collaborator

Having just followed through the process of setting this project up from scratch again, I've made a few corrections to the setup instructions.

More generally, it would be useful to refresh the readme a bit as it explains the project from the perspective of "We've just started this project - these are our ambitions for it" as opposed to "We've been running this project for several years - this is what it does" (spoiler: they're not exactly the same 😉 ), but that is a slightly different job.

- We don't use ruby SASS or yuglify any more
- Correct import commands
- Link to install instructions in README
@ghost ghost assigned chris48s Sep 13, 2018
@ghost ghost added the in progress label Sep 13, 2018
@chris48s
Copy link
Collaborator Author

Note: the reason this PR didn't trigger a build first time was because we need to turn on "Build forked pull requests" in CircleCI. This seems to be off by default (I guess this is because there is an abuse vector here: on a paid plan, someone can just rinse all your build minutes by submitting loads of spurious PRs to your public repos). I've turned it on for all our other projects while I was there, but we'll need to remember to enable it if/when we migrate more projects.

I'll have a look at the coveralls fail now. This is probably also a fix we'll need in lots of places..

@chris48s
Copy link
Collaborator Author

Hmm. As far as I can tell this should work. We've got COVERALLS_REPO_TOKEN correctly defined in the settings :/ I'll continue to chew it over..

@symroe
Copy link
Member

symroe commented Sep 28, 2018

As I'm fairly sure none of these changes are test breaking, should I merge this, or is it useful to keep here for testing circle CI?

@chris48s
Copy link
Collaborator Author

Aah yes I'd forgotten about this.

I'm fairly sure none of these changes are test breaking

If you look at the build, you can see the "run tests" step passed. Its the "Update Coveralls" stage that is failing.

is it useful to keep here for testing circle CI?

I'd quite like to figure this out if we're going to make wider use of Circle CI but I don't think there is anything special about this PR. I expect this will fail with any PR where the source branch is on a fork.. I'm just not sure what the fix is. All the posts I found about this problem said "set COVERALLS_REPO_TOKEN" but we've already done that 🤷‍♂️

@symroe symroe merged commit 1b7e994 into DemocracyClub:master Oct 18, 2018
@ghost ghost removed the in progress label Oct 18, 2018
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

2 participants