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

Make tap configurable #266

Merged
merged 6 commits into from Apr 6, 2020
Merged

Make tap configurable #266

merged 6 commits into from Apr 6, 2020

Conversation

jasonkarns
Copy link
Contributor

This PR is an attempt to make the tap configurable, such that forks may build the website for their own 3rd party taps.

This PR is comprised of two primary changes:

  • Rakefile and generator scripts:
    script/generate.rb and script/generate-cask.rb are modified to accept a tap argument of the form "owner/repo". If the argument is omitted or empty, they default to their present values (CoreTap and default_cask_tap, respectively). To pair with this change, the corresponding Rakefile tasks are also modified to accept a tap argument and to pass it along to the scripts.

  • Tap info in Jekyll configuration
    The second main change in this PR, is the addition of tap information to the Jekyll configuration. These configuration options are then used in the cask and formula templates when constructing URLs for the formulae. Again, the configuration is pre-set to the homebrew-core and cask values.

With both of these changes in place, it becomes easier for the site to be built for 3rd party taps, but still defaults to the primary core/cask taps.

Rakefile Show resolved Hide resolved
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

This looks good, nice work! I wonder if there's any pages like API/analytics etc. that are worth hiding on tap stuff by default?

Rakefile Outdated Show resolved Hide resolved
script/generate-cask.rb Outdated Show resolved Hide resolved
script/generate.rb Outdated Show resolved Hide resolved
jasonkarns and others added 2 commits April 5, 2020 12:23
Co-Authored-By: Mike McQuaid <mike@mikemcquaid.com>
This removes the redundant tap_name logic from the script (which already
exists in homebrew itself). Even better, this will leverage the tap
cache! (And it still preserves the CoreTap singleton preference.)

Also, update task args to use the hash-index form consistently
throughout the rakefile, instead of the (potentially brittle) method
form (which is susceptible to collisions with existing methods).
@jasonkarns
Copy link
Contributor Author

I wonder if there's any pages like API/analytics etc. that are worth hiding on tap stuff by default?

That's a good question!

As you can guess this is related to the discussion on discourse regarding analytics data for 3rd part taps. Taps don't have the ability to generate our own JSON files from GA using the formula-analytics subcommand (due to the GA access restriction). However, as I learned from that thread, tap-specific data is already present in these JSON files on formula.brew.sh. So what I did for my tap's fork of the site was to replace the generate_analytics functions in the Rakefile to curl the corresponding JSON files directly from formula.brew.sh, and run them through a jq filter to select out only the formula that live in my tap.

https://github.com/nodenv/formulae/blob/ae6bc0787539a72d91436f2dc695bcd7de5e23b4/Rakefile#L32-L58

I can't think of a clean way to have logic like this exist in the main site without being super kludgy. (And let's be honest, there aren't going to be many tap-specific forks of this site.) I thought the changes in this PR were simple enough to be worth it; but the kludginess of the analytics stuff is too costly to warrant living in upstream (IMO).

So that accounts for the main event categories (install, install-on-request, cask-install, and build-errors). The rest are already specific to the homebrew-core or linuxbrew, or don't really apply to taps (the macOS version data). So for my tap's fork I just commented them out completely.

I have yet to go through the JSON API documentation page but I assume that one will remain mostly untouched.

The only big remaining piece is the algolia search. I've reached out to algolia to see about setting up a separate account/index for our fork. The only issue is that the algolia configuration lives in the main brew.sh repo. I'm going to experiment with ways to make that configurable, but that's probably beyond the scope of this PR for now.

@MikeMcQuaid
Copy link
Member

I can't think of a clean way to have logic like this exist in the main site without being super kludgy. (And let's be honest, there aren't going to be many tap-specific forks of this site.) I thought the changes in this PR were simple enough to be worth it; but the kludginess of the analytics stuff is too costly to warrant living in upstream (IMO).

Fine with me 👍. Certainly doesn't need to be in this PR.

Shout when you're good to merge (from your perspective)!

Also, if you can be bothered, even some really basic/crappy docs on how to setup a fork of this would be neat.

Thanks again!

@jasonkarns
Copy link
Contributor Author

Latest commit updates core tap references from the api docs page.

The Homebrew/homebrew-core references will appear unchanged. However, the homebrew-core references will now appear as core. Prior to addressing the API docs, all other tap references were either shortname (homebrew/core) or fullname (Homebrew/homebrew-core), so the current yaml config doesn't provide a key for the "full repo name".

If this change is acceptable, I consider this PR ready to merge at this point. I'll work on a separate PR that handles the {{ site.baseurl }} on the API docs page, and includes some docs for tap-specific forks.

@MikeMcQuaid MikeMcQuaid merged commit b578ad7 into Homebrew:master Apr 6, 2020
@MikeMcQuaid
Copy link
Member

Thanks so much for your contribution! Without people like you submitting PRs we couldn't run this project. You rock, @jasonkarns!

@MikeMcQuaid
Copy link
Member

If this change is acceptable, I consider this PR ready to merge at this point. I'll work on a separate PR that handles the {{ site.baseurl }} on the API docs page, and includes some docs for tap-specific forks.

Looking forward to it!

@MikeMcQuaid
Copy link
Member

https://nodenv.github.io/formulae/ works surprisingly well, nice work! Makes me even more glad I decided to hack build this on GitHub Pages!

@jasonkarns
Copy link
Contributor Author

@MikeMcQuaid For sure, Mike! I maybe would still have attempted a fork if it were on heroku, but any other amount of work, I seriously doubt it. GitHub Pages is perfect.

Thanks for all your work!

@jasonkarns jasonkarns deleted the tap branch April 18, 2020 17:29
@lock lock bot added the outdated label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants