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

Allow detecting archived or disabled repos #176

Merged
merged 2 commits into from
Jan 15, 2020
Merged

Allow detecting archived or disabled repos #176

merged 2 commits into from
Jan 15, 2020

Conversation

fingolfin
Copy link
Contributor

This allows listing just "active" repositories in an organization. Right now, we have to manually exclude any such repository.

@fingolfin
Copy link
Contributor Author

Huh, I don't understand those Travis errors... "uninitialized constant Faraday::Error::ClientError" ... ? Is this something caused by my PR, or a wider problem with the Travis tests?

@ashmaroli
Copy link
Member

I don't understand those Travis errors

Please don't worry about the errors. It is caused by a second-order dependency (octokit > faraday) of this plugin. The error will resolve automatically once a new version of octokit gets released..

@ashmaroli
Copy link
Member

@fingolfin The unrelated Faraday errors have been removed from this branch. The current failures are related to this PR. When possible, please look into getting those tests to pass again.

@fingolfin
Copy link
Contributor Author

I had to fix script/webmock-repopulate and re-run it to get up-to-date webmocks. Let's see if that helps CI.

@fingolfin
Copy link
Contributor Author

OK, that causes tons of errors in spec files.

Not sure how best to proceed. I could update those spec files of course (arguably, it would make sense to use up-to-date version of that JSON data). Or I could revert that part of the PR, and just hand-edit the webmock files to have the right keys in them. But I wonder if basing the tests on ancient versions of the JSON produced by GitHub is a good idea?

@ashmaroli
Copy link
Member

I would suggest that you revert all changes that affect tests outside the scope of this pull request first and then tackle the failing tests incrementally..

@fingolfin
Copy link
Contributor Author

That is the second option I listed: I could hand-edit the relevant JSON files to insert the required keys. That's easy enough to do, but seems rather... brittle to me. Is that really what you want?

I could also only update those webmock JSON files (by running script/webmock-repopulate) which are "needed" for testing the archived and disabled repos; but that doesn't substantially simplify the issue, I am afraid.

@ashmaroli
Copy link
Member

@fingolfin You had two two options and asked for the best way to proceed.
I pointed you to the option that results in the most minimal diff for this PR which has a greater chance of being accepted for merge.
Now if you want to debate on or counter that suggestion, all that's going to lead to is noise and a stalemate...

IMO, the best way out is still to revert changes outside the scope of this PR and attempt to get the tests passing by manually editing spec/spec_helpers/integration_helper.rb

@fingolfin
Copy link
Contributor Author

I did not want to debate, I just wanted to verify I understood what you were suggesting, to make sure I don't waste time on an approach that then is rejected later on shrug

@fingolfin
Copy link
Contributor Author

All tests passed now except for one one AppVeyor, which I don't understand, as it complains about a mismatch for latest_release, but I didn't touch that.. It says:

...
Failures:

  1) integration into a jekyll site contains the correct latest_release
     Failure/Error: expect(subject[key].to_s).to match value

       expected "false" to match /assets_url/
       Diff:
       @@ -1,2 +1,2 @@
       -/assets_url/
       +"false"
     # ./spec/integration_spec.rb:30:in `block (3 levels) in <top (required)>'

@ashmaroli ashmaroli requested a review from a team January 8, 2020 10:46
Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

@parkr
Copy link
Member

parkr commented Jan 15, 2020

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit bdff693 into jekyll:master Jan 15, 2020
jekyllbot added a commit that referenced this pull request Jan 15, 2020
@jekyll jekyll locked and limited conversation to collaborators Apr 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants