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

[FIX, DOC] Fix badge in README and docs #39

Closed
wants to merge 2 commits into from

Conversation

tsalo
Copy link
Contributor

@tsalo tsalo commented Aug 4, 2020

Closes None.

Changes proposed in this pull request:

@tsalo tsalo marked this pull request as draft August 4, 2020 19:08
@tsalo
Copy link
Contributor Author

tsalo commented Aug 4, 2020

Would you mind enabling the circleci-artifacts-redirector and CircleCI Checks GitHub apps on this repo? They should help with the status checks.

circleci-artifacts-redirector will add a new status check pointing to the documentation build's artifacts, so you can browse the documentation site before merging.

CircleCI Checks should split the overall CircleCI workflow into separate job-specific status checks, which should let you set up branch protection rules so that you don't merge PRs that fail tests, for example, without restricting based on the style check. You can always force those PRs through if you want, but the extra check should help.

@bbfrederick
Copy link
Owner

As far as I can tell, they are enabled (I know circleci-artifacts-redirector is). The other one looks like it's enabled, but following the directions seems to just keep taking me back to a GitHub page saying I need to enable it on CircleCI...

@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

Merging #39 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #39   +/-   ##
=======================================
  Coverage   17.50%   17.50%           
=======================================
  Files          50       50           
  Lines       14209    14209           
=======================================
  Hits         2487     2487           
  Misses      11722    11722           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd4199d...a16db58. Read the comment docs.

@tsalo
Copy link
Contributor Author

tsalo commented Aug 4, 2020

I'm sorry getting these things set up is such a slog. I've done it a few times, but every time it's like I have to relearn it.

When you go to this repository's Settings --> Integrations --> circleci-artifacts-redirector, and hit the Configure button, it should take you to the "Installed GitHub Apps" tab in your account settings.

When you scroll down to the "Repository access" section, does it have rapidtide selected, either via "All repositories" or listed specifically under "Only select repositories"?

@bbfrederick
Copy link
Owner

bbfrederick commented Aug 7, 2020

No need to apologize! I really appreciate the effort getting this running, as I clearly don't know how to do it...

In answer to your question, yes. According to the Installed GitHub App circleci-artifacts-redirector page, it has access to all repositories.

It does look like all the tests are running - it's just that it's failing the style check, and it's upset about that. You mentioned that I could make is so the style check didn't count as a "real" test, right?

@tsalo
Copy link
Contributor Author

tsalo commented Aug 7, 2020

In answer to your question, yes. According to the Installed GitHub App circleci-artifacts-redirector page, it has access to all repositories.

Okay... I'll try closing this PR and reopening it. Maybe that will trigger it?

It does look like all the tests are running - it's just that it's failing the style check, and it's upset about that. You mentioned that I could make is so the style check didn't count as a "real" test, right?

Once the individual jobs in the CircleCI workflow show up as separate status checks here (which CircleCI Checks should do), you can designate some as "required". Required status checks block PRs from being merged if they fail, although you, as the owner of the repo, can choose to ignore the blocking status checks and merge anyway. The benefit is that you can easily see why things are failing without having to open any separate pages.

Check out this PR I have open for tedana: ME-ICA/tedana#591.
We have one required status check, merge_coverage, which builds off of most of the other jobs within the CircleCI workflow. You'll also notice that all of those individual jobs show up as status checks. That's the thing that should be happening here.

There's also a status check that renders the documentation. The exact method is slightly different in tedana (i.e., it uses a GitHub webhook directly from ReadTheDocs, while here we are using circleci-artifacts-redirector). The idea there is that you can just follow that status check's link and view the documentation as it would render on the site after merging the PR.

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.

2 participants