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

add CircleCI build and docs #12

Merged
merged 2 commits into from
Feb 14, 2019
Merged

add CircleCI build and docs #12

merged 2 commits into from
Feb 14, 2019

Conversation

svenevs
Copy link
Member

@svenevs svenevs commented Feb 8, 2019 •
edited

Here we go! Took a little while to iron out the kinks, but parameters with CircleCI are pretty nice!

Probably easiest to read the instructions rendered on github here. In particular, please review the managing new releases section. I'm not exactly happy with it, but I couldn't find any way to actually detect the cron build.

Action items:

  • Create DOCKER_USERNAME and DOCKER_PASSWORD environment variables through CircleCI thing.
    • Can the pandoc user be pushing? @tarleb do you have that information? I'm totally ok with using my account, but right now I still don't have push access to pandoc/latex. I did trigger a push to pandoc/core:edge though. just gonna push with my user / pw, can be changed in future by admins in CircleCI Environment Variables section.

After this merges, I would like to test the proposed release=2.6 in a commit message (but won't need to run the patching script). However, now that this is live, maybe before we iron out release=2.6 we see if #6 can be done? (adding new comment there for idea I had)

Notes:

  • Any commit that hits master (unless it has a [ci skip] or similar that CircleCI skips) will trigger a docker push.
  • Any pull request triggers the build, it just doesn't push.
  • Current cron schedule is for the first of every month. Please double check (search monthly in .circleci/config.yml), I'm not sure if I did that right.
  • I won't be offended if we decide to gut the README docs I've added here, maybe it gets too redundant? The patching script was quick to write. NBD if it is better not to use it ;)

@tarleb
Copy link
Member

tarleb commented Feb 8, 2019

right now I still don't have push access to pandoc/latex

My bad (although I thought I'd done that – oh well). Should be fixed now.

@svenevs
Copy link
Member Author

svenevs commented Feb 8, 2019

Ok I will test push after work tonight I hope if not tomorrow.

I'm looking at the changes here and I think they're ridiculous. Will be restructuring the README, removing the python script, and only making note of current latest tag in one location. That will make things much more maintainable.

I will also try and fix the "detect Cron build", a colleague told me something I thought I tried should work.

@tarleb
Copy link
Member

tarleb commented Feb 8, 2019

This looks really good! Maybe the PR should be split into two or more, given the number of things it adds.

Would it make sense to store a README template, and then generate the real README from that? This is how pandoc's README is maintained and kept up to date.

@svenevs
Copy link
Member Author

svenevs commented Feb 13, 2019

hey, sorry for falling off the grid on this. had a really bad case of the flu :(

Maybe the PR should be split into two or more, given the number of things it adds.

hopefully you agree this is no longer necessary. Python nonsense and auto patching in favor of a much simpler README.

  1. Adding a new Image Stack is easier. Please understand that though not ideal, this is the best we can get from CircleCI in terms of being able to distinguish cron builds.
  2. As a tradeoff for some minor (but easy) copy-paste action in .circleci/config.yml, the Managing new Pandoc Releases is much easier. It doesn't matter anymore if the release=X.Y commit stays on master, cron jobs are detectable.

Would it make sense to store a README template, and then generate the real README from that? This is how pandoc's README is maintained and kept up to date.

I don't think it's necessary, I'm happy with the simple setup here. By simply linking to the dockerhub repository, all available tags are shown. I'm definitely not opposed, but I think we have bigger fish to fry here :D

I'm changing my dockerhub password now and then adding it to CircleCI for the pandoc build, I think the CI here failed from me force pushing (confused them?).

@svenevs
Copy link
Member Author

svenevs commented Feb 13, 2019

I've since added DOCKER_USERNAME and DOCKER_PASSWORD to pandoc build environment on CircleCI, deleted my svenevs/dockerfiles build from CircleCI (told it to stop building project). But I think I confused CircleCI with force pushing and deleting and stuff? That build status image is from my svenevs/dockerfiles (which is now deleted in attempt to get a pandoc/dockerfiles build). Maybe it's because there's no .circleci/config.yml on master right now?

I'm not sure what to do :( I can setup a shadow repo to "prove this works" if you want a green check before merging. But if I re-enable svenevs/dockerfiles I'm worried about getting even more confused.

tarleb
tarleb approved these changes Feb 14, 2019
View reviewed changes
Copy link
Member

@tarleb tarleb left a comment

Choose a reason for hiding this comment

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

<3

.circleci/config.yml
# The only difference in usage between (1) and (2) is whether or not `cron_job`
# is getting set which leads to some duplication, but at this time there is no
# known solution for fixing this.
alpine_stack: &alpine_stack
Copy link
Member

Choose a reason for hiding this comment

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

nice!

Makefile
@@ -29,3 +29,6 @@ alpine-latex:
--build-arg base_tag=$(PANDOC_VERSION) \
alpine/latex

.PHONY: lint
lint:
shellcheck $(shell find . -name "*.sh")
Copy link
Member

Choose a reason for hiding this comment

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

🙌

README.md Outdated
@@ -1,5 +1,145 @@
pandoc Dockerfiles
==================
# pandoc Dockerfiles
Copy link
Member

Choose a reason for hiding this comment

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

y tho?

Copy link
Member Author

Choose a reason for hiding this comment

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

reverted. it was a remnant of the original README that had h3 headings, which can't be done underline style. but we don't need them anymore and i prefer underline too. I made them all 80 cols just because it makes it easier to read 🙂

@svenevs svenevs merged commit e3530c7 into pandoc:master Feb 14, 2019
@svenevs svenevs deleted the circle-ci branch February 14, 2019 09:52
@tarleb tarleb mentioned this pull request Feb 14, 2019
Closed
5 tasks
@svenevs svenevs mentioned this pull request Feb 14, 2019
Closed
3 tasks
@svenevs
Copy link
Member Author

svenevs commented Feb 14, 2019

WOOHOO! With merge to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tarleb tarleb tarleb approved these changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@svenevs @tarleb