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

Improve setup and seed #1352

Merged
merged 108 commits into from
Oct 30, 2019
Merged

Improve setup and seed #1352

merged 108 commits into from
Oct 30, 2019

Conversation

teadur
Copy link
Contributor

@teadur teadur commented Oct 9, 2019

Retry of #1296
First part of fixing #960, install gem deps and setup db in bin/setup.
Install gem build deps in bin/setup.deps.
Provide db:seed with initial data.

New codeclimate issues are result of rubocop-i version increase not my code changes, because of that i think it makes more sense to merge the PR and work on the new issues gradually not in this PR.

@teadur teadur requested a review from artur-intech October 9, 2019 22:46
Copy link
Contributor

@artur-intech artur-intech left a comment

Choose a reason for hiding this comment

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

Can you describe the point of having bin/setup.deps? Is it so that setup will be used for development instances, and bin/setup.deps for production-like ones?

.deps is quite unusual. Can we find some more descriptive name, like setup_prod? (given I was correct. But then I wonder if we really need two versions. Would be good to understand the motive of having this script).

bin/setup.deps Outdated
system(*args) || abort("\n== Command #{args} failed ==")
end

def get_uid()
Copy link
Contributor

Choose a reason for hiding this comment

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

effective_user_id might explain the intention better. () can be skipped.

bin/setup.deps Outdated

def get_uid()
uid = `id -u`
uid.delete!("\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

bin/setup.deps Outdated Show resolved Hide resolved
bin/setup.deps Outdated

puts "== Installing rbenv ruby manager to #{ENV['HOME']} =="
unless Dir.exist?("#{ENV['HOME']}/.rbenv/")
system! 'git clone https://github.com/sstephenson/rbenv.git $HOME/.rbenv'
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps https://github.com/rbenv/rbenv.git looks more official? This way you don't need to figure out who is that "sstephenson" every time you see this.

bin/setup.deps Outdated

# Include RBENV in path
ENV['PATH'] = ENV['HOME'] + "/.rbenv/bin:" + ENV['HOME'] + "/.rbenv/plugins/ruby-build/bin:" + ENV['PATH']
NEEDED_RUBY = `cat .ruby-version`.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

NEEDED_RUBY_VERSION is more intention revealing.

bin/setup.deps Outdated
ENV['PATH'] = ENV['HOME'] + "/.rbenv/bin:" + ENV['HOME'] + "/.rbenv/plugins/ruby-build/bin:" + ENV['PATH']
NEEDED_RUBY = `cat .ruby-version`.freeze

if '!bundle check' == false
Copy link
Contributor

Choose a reason for hiding this comment

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

'!bundle check' == false is always false. Did you mean backtick instead of a single quote?

db/seeds.rb Outdated
roles: ['admin']
)
# Required for creating registrar
Setting.where('var': 'registry_vat_prc').first_or_create(
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not figure out how to load defaults variables from app.yml it isn't done automatically

db/seeds.rb Outdated
value: '0.2'
)
# First registrar
Registrar.where('name': 'Registrar First').first_or_create!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to create a registrar? Given you create an AdminUser on line 5, he can login by itself and create registrars by himself. Unlikely anybody will benefit from such registrar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to have demo data set there to ease the testing of features in development

Copy link
Contributor

Choose a reason for hiding this comment

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

rake db:fixtures:load should be used for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dummy data is already available in test/fixtures.

code: 'REG1'
)

# registrar.accounts.create!(account_type: Account::CASH, currency: 'EUR')
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove it entirely if not needed. Git history has it anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They will be enabled in future commits

db/seeds.rb Outdated
@@ -1,2 +1,59 @@
# This file should contain all the record creation needed to seed the database with its default values.
# The data can then be loaded with the rake db:seed (or created alongside the db with db:setup).
# The data can then be loaded with the rake db:seed (or created alongside the db with db:setup).
ActiveRecord::Base.transaction do
AdminUser.where('username': 'admin').first_or_create!(
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be username: 'admin' (:username is a symbol).

@artur-intech
Copy link
Contributor

artur-intech commented Oct 11, 2019

@teadur Wouldn't it be better to update Rubocop in a separate PR? It's kinda confusing why there are so many CodeClimate issues.

At the very minimum it should be separate commit. Currently there are way too many unrelated changes in a single commit (e70fa4c). If we need to do something with it (revert, cherry-pick, etc), we're in trouble.

vohmar and others added 23 commits October 24, 2019 20:03
@teadur
Copy link
Contributor Author

teadur commented Oct 29, 2019

@artur-beljajev rebased on master and will be squash merged to master.
@timo want todo it yourself or shall I ?

@teadur teadur merged commit 6fb8767 into master Oct 30, 2019
@teadur teadur mentioned this pull request Oct 31, 2019
teadur added a commit that referenced this pull request Nov 1, 2019
fix typos introduced in #1352
@teadur teadur deleted the improve-setup-and-seed branch September 16, 2020 07:15
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