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

[INFRA] Link to doc builds in CI checks #315

Merged
merged 1 commit into from
Sep 6, 2019

Conversation

jasmainak
Copy link
Collaborator

This PR introduces a quick link to the specification when a PR is made. To enable this, we would have to install this app: https://github.com/larsoner/circleci-artifacts-redirector. It was developed by @larsoner from the MNE team. If people agree, I think it can really help in reviewing PRs here.

@larsoner
Copy link

You'll also need to change your doc build name to be build_doc instead of build, see the caveats in the readme of https://github.com/larsoner/circleci-artifacts-redirector

It would be better to enhance the app to add config options for this sort of thing, but JavaScript is not my forte...

@effigies
Copy link
Collaborator

Sounds reasonable to me.

@effigies
Copy link
Collaborator

@jasmainak Do you want to add a commit to test it out?

@@ -1,6 +1,6 @@
version: 2
jobs:
build:
build_docs:

Choose a reason for hiding this comment

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

BTW I pushed a commit to circleci-artifacts-redirector that made it so that build is okay, too. But build_docs might be a clearer name anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!

@jasmainak
Copy link
Collaborator Author

@effigies I squashed the two commits to force a rebuild. You can check out the quicklink now.

@effigies
Copy link
Collaborator

LGTM: Screen Shot 2019-08-21 at 16 42 44

@effigies effigies changed the title DOC: add quick link to built specification [INFRA] Link to doc builds in CI checks Aug 21, 2019
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

wonderful, thanks @larsoner and @jasmainak

@KirstieJane
Copy link
Member

Super cool! I love this.

Is there a way to make the fact that the docs have been rendered a little clearer? I’m not sure folks will know to click on the 3rd CI test in a group of 4.

Netlify has a bot that comments for you when the link is ready, but if that doesn’t exist maybe we could add a comment in the PR template? Something like “Click on the build_docs artifact link from circleci to see how your changes will appear in the specification”?

No worries if this is too hard, it’s a super useful contribution!!

@larsoner
Copy link

It's possible in principle at the app end. The trade-off is that multiple comments per PR becomes spam-like over time with many commits. To reduce it (change to one comment per PR that gets edited) hurts discoverability/usability probably even worse than a commit status.

But in the end the blocker is really just that someone (not me!) needs to volunteer to code the functionality, and make it configurable. It's JavaScript and the GitHub API is actually pretty simple if someone wants to try!

@jasmainak
Copy link
Collaborator Author

@KirstieJane can you point us to the netlify bot that does that?

My first thought was also that the app should comment with every commit but I agree it can produce a bit of spam. Or perhaps it should look at the diff when making the PR and point to the file with the maximum number of lines changed. I think all this is possible with javascript but I didn't want to volunteer with my limited javascript knowledge :) But it sounds like a fun hackathon project.

@yarikoptic
Copy link
Collaborator

yarikoptic commented Aug 22, 2019

I am confused, what extra in the last comments you would like on top of that nice

ci/circleci: build_docs artifact
Link to 0/home/circleci/project/site/01-introduction.html

which is how present in the status for the every build?

Edit: I would vote to not add more of bot comments since indeed they would occlude possible discussions.

@KirstieJane
Copy link
Member

KirstieJane commented Aug 22, 2019

@jasmainak: here’s a PR with an example of the netlify bot commenting: the-turing-way/the-turing-way#613

@larsoner: I see your point about editing all the time. My experience has been that the edits are quite smooth because I think it keeps appearing at the bottom? I think it’s probably a UI win that I don’t think about it so I can’t answer your question properly! 😂🤣😂

@yarikoptic: I don’t think the link is very discoverable, so my suggestion was to highlight the work that has been implemented.

No need to implement anything new. Certainly not your responsibility @larsoner!! I think noting it in the PR template is probably the easiest way to help folks know that the spec is built already 🌸

(Edited to actually include the link 🤦🏼‍♀️ and clarify that the bot comment keeps changing at the top not the bottom of the PR)

@effigies
Copy link
Collaborator

effigies commented Sep 6, 2019

I'm seeing we didn't merge this. Given that this is an improvement as-is, and there's not a lot of motion to try to improve right now, I'm going to merge. I've created an issue (#326) to update the PR template and/or move to a top-commenting + editing bot like netlify.

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.

6 participants