GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Migrate from Travis CI to GitHub Actions by etagwerker · Pull Request #203 · fastruby/fast-ruby

Conversation

etagwerker commented Jul 7, 2022

Copy link

Copy Markdown

Member

Hi there,

This PR fixes #202 and adds sharing as a default for the rake task. Sharing is set up to only run in CI, so that it doesn't run on development environments when people are trying to make a contribution to this project...

Please check it out.

Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Enable sharing to easily share results online

8f48ef1

Only share when running within GitHub Actions environment.

etagwerker requested a review from arielj

July 7, 2022 08:55

etagwerker force-pushed the feature/migrate-to-github-actions branch 2 times, most recently from 0141b11 to 77d327a Compare

July 7, 2022 09:00

ixti approved these changes

Jul 7, 2022

View reviewed changes

ixti reviewed

Jul 7, 2022

View reviewed changes

Comment thread

Rakefile Outdated

		@@ -1,5 +1,7 @@
		desc "run benchmark in current ruby"
		task :run_benchmark do
		ENV['SHARE'] = ENV['GITHUB_ACTIONS'] # only share to http://ips.fastruby.io when running within GitHub Actions env

ixti Jul 7, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You can export SHARE env var in the GH actions workflow instead

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

etagwerker reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

.github/workflows/rake.yml Outdated

		# This workflow uses actions that are not certified by GitHub.
		# They are provided by a third-party and are governed by
		# separate terms of service, privacy policy, and support
		# documentation.

ixti Jul 7, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

IMO these comments are mostly confusing than helping.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

etagwerker reacted with thumbs up emoji

All reactions

👍 1 reaction

etagwerker commented Jul 7, 2022

Copy link

Copy Markdown

Member Author

@ixti Thanks for your review! I addressed your suggestions and updated my changes. 👍

ixti reacted with heart emoji

All reactions

❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

arielj approved these changes

Jul 11, 2022

View reviewed changes

arielj left a comment

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks good to me.

Just a suggestion but shouldn't be a blocker: maybe rename the workflow to something like Benchmarks and benchmarks.yml, currently it's Ruby CI an the filename is rake.yml.

One question that might be outside of the scope of this PR, I see the benchmarks are shared at ips.fastruby.io, mabye this should happen only if the tests are run on the main branch? I imagine sharing results for every PR change might be a lot, but I don't know how ips.fastruby.io work (maybe this behavoir is intentional, just asking)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

arielj commented Jul 11, 2022

Copy link

Copy Markdown

Collaborator

One more thing I forgot, can you update the README file removing the reference to Travis CI? Maybe link to the Actions index.

etagwerker reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

etagwerker mentioned this pull request

Jul 11, 2022

[FEATURE REQUEST] Use the project's WIKI to share results for each Ruby version with latest CI results #206

Open


          Replace Travis CI w/ GitHub Actions CI

15b44d5

There are a few things that had to happen:

- Had to add more modern rubies to the CI configuration.

- Had to tweak versions to use the latest patch levels and bump jruby

GitHub Actions doesn't have some of the versions that we were using in Travis CI.

- Update README to point to GitHub Actions instead of Travis CI

etagwerker force-pushed the feature/migrate-to-github-actions branch from 3fcc00e to 15b44d5 Compare

July 11, 2022 14:38

etagwerker commented Jul 11, 2022

Copy link

Copy Markdown

Member Author

Just a suggestion but shouldn't be a blocker: maybe rename the workflow to something like Benchmarks and benchmarks.yml, currently it's Ruby CI an the filename is rake.yml.

Sounds good to me.

One question that might be outside of the scope of this PR, I see the benchmarks are shared at ips.fastruby.io, mabye this should happen only if the tests are run on the main branch?

@arielj I like this idea, but this can be a future enhancement. This repo doesn't get a ton of PRs so I'm not worried about sharing it with every PR for now. Once this PR is merged I can create an issue so we don't lose track of this.

One more thing I forgot, can you update the README file removing the reference to Travis CI? Maybe link to the Actions index.

Sure thing, done!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

etagwerker merged commit 9eb1218 into main

Jul 12, 2022

etagwerker deleted the feature/migrate-to-github-actions branch

July 12, 2022 07:28

etagwerker mentioned this pull request

Jul 12, 2022

[CHANGE] Share of results should only happen when running GitHub Actions in the main branch #207

Closed

etagwerker commented Jul 12, 2022

Copy link

Copy Markdown

Member Author

@arielj Just merged this PR and added a new issue to address the sharing enhancement: #207 👍

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Reviewers

ixti

ixti approved these changes

arielj

arielj approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Run all the benchmarks on CI

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.