Skip to content

Conversation

wilson
Copy link
Contributor

@wilson wilson commented Jul 26, 2018

No description provided.

@wilson wilson requested a review from maxjacobson July 26, 2018 14:21

ed Gemfile <<-EDITS
/gem "parser",/c
gem "parser"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this replacing the line with the same line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's replacing the version-specific line on master with one that doesn't specify a version. Master is pinned to an oddly-old parser, it seemed to me.

gem "rubocop", "~> ${NEWVER}", require: false
.
wq
EDITS
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL some ed basics, that's neat 👍

bin/newver Outdated
wq
EDITS

bundle update
Copy link
Contributor

Choose a reason for hiding this comment

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

should we do something more scoped like, bundle update rubocop?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: in order for this to work, it would be necessary to add a make image before the ed line

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops, this comment was meant to go on the thread about using docker, not the thread about scoping the bundle update 😅

bin/newver Outdated
wq
EDITS

bundle update
Copy link
Contributor

Choose a reason for hiding this comment

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

wdyt about doing this in docker? In general we try to avoid needing to have non-docker things installed on your host. We could do something like:

docker run --user root --rm --volume "$(pwd)":/usr/src/app --workdir /usr/src/app codeclimate/codeclimate-rubocop bundle update rubocop

In a lot of our code bases, we've added such a task to the Makefile as make bundle. If we did that, we might invoke it here as make bundle -e BUNDLE_ARGS='update rubocop'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems like a nice improvement; I will change it to invoke docker like your example.

retval=$?
if [ $retval -ne 0 ]; then
>&2 echo "If tests fail, you may need to modify spec/support/currently_undocumented_cops.txt"
exit $retval
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a nice CLI UX 👏

* Make bundle uses docker, Following up on PR review
and to be consistency with how we update gems in other
repos
@ale7714
Copy link
Contributor

ale7714 commented Oct 22, 2018

@maxjacobson I updated the script. I think it would be nice to merge this because it helps with releasing new versions. Can i get another review?

@ale7714 ale7714 merged commit 9d00c5e into master Oct 22, 2018
@ale7714 ale7714 deleted the wlb/new_version_script branch October 22, 2018 22:58
ale7714 pushed a commit that referenced this pull request Oct 23, 2018
* Add `bin/newver` script for creating new Rubocop channels.

* Adds make bundle and updates newver script

* Make bundle uses docker, Following up on PR review
and to be consistency with how we update gems in other
repos
ale7714 pushed a commit that referenced this pull request Oct 24, 2018
* Add `bin/newver` script for creating new Rubocop channels. (#129)

* Add `bin/newver` script for creating new Rubocop channels.

* Adds make bundle and updates newver script

* Make bundle uses docker, Following up on PR review
and to be consistency with how we update gems in other
repos

* Updates rubocop-rspec 1.30
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