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

chore: add circleci config #113

Merged
merged 13 commits into from
Dec 29, 2021
Merged

chore: add circleci config #113

merged 13 commits into from
Dec 29, 2021

Conversation

frederickfogerty
Copy link
Contributor

@frederickfogerty frederickfogerty commented Dec 24, 2021

This PR adds CI config for CircleCI. The config is essentially the same as the Travis config, except that it doesn't include the Rubium config, since that didn't seem to work on Travis anyway.

@frederickfogerty frederickfogerty requested a review from a team as a code owner December 24, 2021 14:24
@commit-lint
Copy link

commit-lint bot commented Dec 24, 2021

Chore

  • remove Gemfile.lock from .gitignore (1a05850)
  • add Gemfile.lock (4efce90)
  • add circleci config (1ad16f2)
  • remove Travis config (73f819f)
  • update docs about what versions are tested (c40c632)
  • add ruby v3 to CI config (324ae4a)
  • use ruby v3.0 (b17ef7a)
  • use circleci matrix to define different ruby envs (6a4ec39)
  • use version for cache key (987f48b)
  • add comments to circleci file (5ee8ebc)
  • add .circleci to gemspec ignored list (372ef45)
  • remove lockfile (412d2f5)
  • add platform to bundle (6570d72)

Contributors

frederickfogerty

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

@frederickfogerty
Copy link
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

Copy link
Contributor

@sherwinski sherwinski left a comment

Choose a reason for hiding this comment

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

LGTM but just had two points for discussion:

  1. Since migrating from travis isn't so 1-to-1, can we get some descriptive comments about all the newer pieces we're seeing in the circle config, e.g. everything in the install-deps and steps blocks. This would be helpful in the future if a different dev needs to figure their way around this.
  2. Was there a specific motivation for checking in the lock file? I don't disagree with the choice but just wanted to be clear on the context as to why. CC @ericdeansanchez I remember discussing ruby lock files way back, can you confirm we're still good to check them into our libraries.

@frederickfogerty
Copy link
Contributor Author

  • Since migrating from travis isn't so 1-to-1, can we get some descriptive comments about all the newer pieces we're seeing in the circle config, e.g. everything in the install-deps and steps blocks. This would be helpful in the future if a different dev needs to figure their way around this.

I agree that the install-deps command is confusing. Since it is simply just copied from the Ruby orb, I think it's best just to link that documentation, which I have done. I'll add some other comments to add any context I know of.

  • Was there a specific motivation for checking in the lock file? I don't disagree with the choice but just wanted to be clear on the context as to why. CC @ericdeansanchez I remember discussing ruby lock files way back, can you confirm we're still good to check them into our libraries.

It is recommended by Ruby/Rails to check-in the lockfile. If there was a clear decision in the past not do check it in, I'm happy to remove it again.

@frederickfogerty
Copy link
Contributor Author

@sherwinski made some updates here, let me know what you think

@ericdeansanchez
Copy link
Contributor

@frederickfogerty @sherwinski the short answer: it's fine, check it in! It's one of those things that people have opinions about.

The short story is there's been two popular opinions:

  • check-in lock file for apps (reproducibility)
  • do not check-in lock file for gems (flexibility for users)

But there's a third option: check-in the lock file and proactively (routinely) test against the newest version of dependencies.

I vote for this third option.

@ericdeansanchez
Copy link
Contributor

We should add .circleci to the list of things we ignore when distributing this gem, e.g.

`git ls-files -z`.split("\x0").reject { |f| f.match(%r{^(test|spec|features|.github|.circleci)/}) }

Mostly a comment: This dependency scan is wild lol. Here's the gems we use:

require "base64"
require "cgi/util"
require "erb"
require "digest"
require "net/http"
require "uri"
require "json"

@frederickfogerty
Copy link
Contributor Author

Thanks for the feedback @ericdeansanchez. I think I'll remove the lockfile for now, and then we can add it when we add renovate or something else at a later stage.

Copy link
Contributor

@sherwinski sherwinski left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@frederickfogerty frederickfogerty merged commit f600247 into main Dec 29, 2021
@frederickfogerty frederickfogerty deleted the f/circle-ci branch December 29, 2021 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants