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

CI: check yard & mdl output #14156

Merged
merged 2 commits into from Nov 29, 2022

Conversation

EricFromCanada
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

An attempt at having CI test for issues with YARD's rubydoc generation. Doesn't currently work because I'm unsure of how to get bundler working. (Next step after that would be to add Markdown formatting checks using mdl now that it's shipped a release with this fix, although it now requires Ruby 2.7.)

@BrewTestBot
Copy link
Member

Review period will end on 2022-11-18 at 19:55:46 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Nov 17, 2022
@Bo98
Copy link
Member

Bo98 commented Nov 17, 2022

We no longer expose our install of Bundler at all and there's no dev-cmd entry point beyond what brew vendor-gems offers at the moment.

Though a Bundler installation from an external Ruby via ruby/setup-ruby is probably sufficient. Will probably install gems to the vendor directory given the .bundle/config but that's ok since we're not committing anything here.

If any Bundler 2.3 is available (which I think ruby/setup-ruby might provide for Ruby 2.6, 2.7 and 3.1), Bundler will actually auto-download and install the version of Bundler specified in the lockfile.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
Library/Homebrew/Gemfile Outdated Show resolved Hide resolved
docs/Rakefile Show resolved Hide resolved
@EricFromCanada EricFromCanada added the help wanted We want help addressing this label Nov 18, 2022
@EricFromCanada EricFromCanada changed the title CI: check yard output CI: check yard & mdl output Nov 18, 2022
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Nov 18, 2022
@BrewTestBot
Copy link
Member

Review period ended.

.github/workflows/docs.yml Outdated Show resolved Hide resolved
@EricFromCanada EricFromCanada force-pushed the additional-ci-tests branch 4 times, most recently from 8522085 to 479000f Compare November 19, 2022 22:49
@EricFromCanada EricFromCanada removed the help wanted We want help addressing this label Nov 19, 2022
@EricFromCanada EricFromCanada marked this pull request as ready for review November 19, 2022 22:54
@EricFromCanada
Copy link
Member Author

Seems to be working now. For now it won't hold up any future PRs until these steps are enabled:

  • "Check Markdown syntax": pending some final fixes for the last few docs.brew.sh sections (coming soon)
  • "Process rubydoc comments": needs someone to track down how to either fix the error and two warnings that YARD produces, or have them be skipped

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.

Great work here! If/when you merge this: please open another draft PR with the broken YARD stuff and I'll attempt to figure it out and push to it.

.github/workflows/docs.yml Outdated Show resolved Hide resolved
.github/workflows/docs.yml Outdated Show resolved Hide resolved
.github/workflows/docs.yml Outdated Show resolved Hide resolved
.github/workflows/docs.yml Outdated Show resolved Hide resolved
BUNDLE_GEMFILE: ../../rubydoc/Gemfile
run: |
bundle install --jobs 4 --retry 3
bundle exec yard doc --plugin sorbet --no-output #--fail-on-warning
Copy link
Member

Choose a reason for hiding this comment

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

I reckon it's maybe work splitting this into a separate job given it's working on a different Bundler? Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you may be overestimating my knowledge of bundler, gems, and Ruby in general… 🙃

Copy link
Member

Choose a reason for hiding this comment

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

@EricFromCanada Sorry, I mean the two differing Gemfile/BUNDLE_GEMFILE bits here may make sense to be in different GitHub Actions jobs in the same file. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

They're now in separate jobs. Only outstanding item is the Test Everything check which seems to be unrelated.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
@EricFromCanada EricFromCanada force-pushed the additional-ci-tests branch 2 times, most recently from e548999 to fb12079 Compare November 26, 2022 15:25
With fixes for Markdown syntax to allow tests to pass.
@MikeMcQuaid MikeMcQuaid merged commit f3085e5 into Homebrew:master Nov 29, 2022
@MikeMcQuaid
Copy link
Member

Thanks again @EricFromCanada!

@EricFromCanada EricFromCanada deleted the additional-ci-tests branch November 30, 2022 05:04
@github-actions github-actions bot added the outdated PR was locked due to age label Dec 31, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants