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

[INFRA] Use linkchecker (from a dedicated docker image) to check all URLs #293

Merged
merged 13 commits into from
Aug 12, 2019
Merged

[INFRA] Use linkchecker (from a dedicated docker image) to check all URLs #293

merged 13 commits into from
Aug 12, 2019

Conversation

yarikoptic
Copy link
Collaborator

@yarikoptic yarikoptic commented Aug 2, 2019 •
edited
Loading

Supersedes #79

TODOs

  • make sure it works
  • possibly change tag and transfer docker image under bids-standard organization on hub.docker.org
  • fix broken internal (anchored) URLs
  • add --check-extern option
  • fix broken external URLs
  • fixup what broke (thanks travis)

@yarikoptic yarikoptic force-pushed the bf-links-docker branch 4 times, most recently from bf13283 to 349e386 Compare August 5, 2019 13:47
@yarikoptic yarikoptic force-pushed the bf-links-docker branch 3 times, most recently from 5f8146d to f46ab03 Compare August 5, 2019 14:10
@yarikoptic
Copy link
Collaborator Author

@sappelhoff WOOHOO -- I finally won (and learned a bit of circle-ci spec ;)). Now I will fixup broken URLs to make it all green!

@yarikoptic
BF: add linkchecker command (entered image), and chmod entire ~ to be…
64faa85 
… readable -- demanded by linkchecker

- note: it is important to have trailing / after site/ for linkchecker
@yarikoptic
BF: fixed up #anchored references in two files
f795519
@yarikoptic
BF(workaround): run linkchecker in a single thread
002c365 
apparently my anchor fix to linkchecker is not complete.  In multithreaded mode
(the beauty of linkchecker) it brings up false positives reporting some anchors
which are valid.  See e.g.

https://circleci.com/gh/bids-standard/bids-specification/911?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link
@yarikoptic
BF: URL to mindboggle should have no www.
7d329ff
@sappelhoff
Copy link
Member

super cool, thanks for investing the effort! This looks really good and your to-dos seem reasonable.

+1 to migrate the dockerimage to "bids": https://hub.docker.com/u/bids

@yarikoptic
Copy link
Collaborator Author

+1 to migrate the dockerimage to "bids": https://hub.docker.com/u/bids

should I just try to push there using the same name:tag or any recommendations ? (not sure if I have super powers for that, but can try)

@yarikoptic
Copy link
Collaborator Author

WOOHOO #2 -- all green! so I am done with this, besides may be pushing docker image to under bids/ on docker hub. Unfortunately the solution is not ideal (requires more fixups to linkchecker so threading is disabled) but seems to work.
If you see ways to improve -- you are welcome to push directly into my branch, I need to switch to other things todo.

sappelhoff
sappelhoff previously approved these changes Aug 5, 2019
View reviewed changes
@yarikoptic
Copy link
Collaborator Author

ok, conflicts are piling up -- review/instructions from others to see this merged would be appreciated!

@yarikoptic
Copy link
Collaborator Author

attn: @sappelhoff

+1 to migrate the dockerimage to "bids": https://hub.docker.com/u/bids

should I just try to push there using the same name:tag or any recommendations ? (not sure if I have super powers for that, but can try)

@yarikoptic
Merge remote-tracking branch 'origin/master' into bf-links-docker
4432d90 
* origin/master:
  [DOC] Auto-generate changelog entry for PR bids-standard#272
  Apply suggestions from code review
  reorder sections as chris suggested
  STY: Move to passive voice/imperative mood
  consistent use of <label>
  Apply suggestions from code review
  right align cells and formtting
  add general section on entities
  fix linter
  some cleanup/clarifying
  try links via tiny headings
  try fix links
  try links
  transpose entity table
@sappelhoff
Copy link
Member

I was leaving this open for a bit to see if other people want to review this @yarikoptic :-)

I have one open question: What is the point of tools/linkchecker-docker/neurodocker-build.sh?

@yarikoptic
Copy link
Collaborator Author

I was leaving this open for a bit to see if other people want to review this @yarikoptic :-)

I have one open question: What is the point of tools/linkchecker-docker/neurodocker-build.sh?

that is how that docker image recipe (Dockerfile) is built -- using neurodocker.
Benefits -- could be multiple, e.g. with slight tune up I could also build Singularity recipe, and neurodocker's recipe takes care about cleaning things up thus minimizing the image/layers.
I guess I could/should just save it and commit so people could build their own image without requiring neurodocker being installed?

@sappelhoff
Copy link
Member

that is how that docker image recipe (Dockerfile) is built -- using neurodocker.

okay, I had suspected something like this --> so is your docker image (linkchecker) calling this script? Because I don't see circleci calling it at any time 🤔

I guess I could/should just save it and commit so people could build their own image without requiring neurodocker being installed?

Could you please clarify what you mean by that?

@yarikoptic
Copy link
Collaborator Author

My image is specified for that run in circle ci config https://github.com/bids-standard/bids-specification/pull/293/files#diff-1d37e48f9ceff6d8030570cd36286a61R34

Now it is just pipped into docker build, so never saved. I could save it to a file and commit it first

@sappelhoff
Copy link
Member

My image is specified for that run in circle ci config

yes, but according to what I understand, this tells CircleCi to download your image from dockerhub

Now it is just pipped into docker build, so never saved. I could save it to a file and commit it first

I think I just don't understand why we need to add the build of the docker image to this repo 😕 ... what would break if we simply removed neurodocker-build.sh from the new tools/linkchecker-docker directory that you created?

@yarikoptic
Copy link
Collaborator Author

Now it is just pipped into docker build, so never saved. I could save it to a file and commit it first

I think I just don't understand why we need to add the build of the docker image to this repo ... what would break if we simply removed neurodocker-build.sh from the new tools/linkchecker-docker directory that you created?

it is not building docker image here on CI or travis anywhere. It is just the recipe on HOW that image could be built happen we need to update it (e.g. to a new patched or not version of linkchecker). without it, there would be no clear way on how to do that

@sappelhoff
Copy link
Member

it is not building docker image here on CI or travis anywhere. It is just the recipe on HOW that image could be built happen we need to update it (e.g. to a new patched or not version of linkchecker). without it, there would be no clear way on how to do that

Ahhh it's what you did prior to pushing the image to your dockerhub ... I got it. Thanks for explaining :-)

Now I'd be ready to merge this PR, but you mentioned something along the lines

I guess I could/should just save it and commit so people could build their own image without requiring neurodocker being installed?

--> My take on this is: People who would do this kind of stuff would have no issues getting neurodocker first ... so from my side, everything is fine. :-)

@sappelhoff
Copy link
Member

I don't want to delay this PR any further. All future improvements can be done in a separate PR,

this is a nice enhancement as it stands ... thank you very much @yarikoptic!

@sappelhoff sappelhoff merged commit fc2638c into bids-standard:master Aug 12, 2019
franklin-feingold added a commit that referenced this pull request Aug 12, 2019
@franklin-feingold
[DOC] Auto-generate changelog entry for PR #293
3f9dd7d
@sappelhoff sappelhoff mentioned this pull request Aug 12, 2019
Closed
@yarikoptic
Copy link
Collaborator Author

THANKS!

@sappelhoff sappelhoff mentioned this pull request Apr 6, 2020
Closed
@sappelhoff sappelhoff changed the title [INFRA+FIX] Use linkchecker (from a dedicated docker image) to check all URLs [INFRA] Use linkchecker (from a dedicated docker image) to check all URLs Jul 27, 2022
@yarikoptic yarikoptic mentioned this pull request Apr 19, 2024
Merged
@yarikoptic yarikoptic deleted the bf-links-docker branch April 30, 2024 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sappelhoff sappelhoff sappelhoff approved these changes

@chrisgorgo chrisgorgo Awaiting requested review from chrisgorgo

@effigies effigies Awaiting requested review from effigies

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

Successfully merging this pull request may close these issues.
2 participants
@yarikoptic @sappelhoff