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

Add the evm:deployment_status rake task #15402

Merged
merged 2 commits into from
Jun 21, 2017

Conversation

carbonin
Copy link
Member

This is mainly for running within containers where we want to know if we require initialization work to be done and if we should migrate the database.

Previously this was done from (untested) bash scripts.

Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

Overall, looks good 👍 just a test comment.

end

it "returns new_replica if the current server is not seeded" do
server.delete
Copy link
Member

Choose a reason for hiding this comment

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

It feels awkward to create the server then delete it. I'd rather see the server created only in the test cases where it is needed.

This is mainly for running within containers where we want to know
if we require initialization work to be done and if we should
migrate the database.

Previously this was done from (untested) bash scripts.
@carbonin carbonin force-pushed the add_deployment_status_rake_task branch from f673210 to 7ab13ea Compare June 19, 2017 22:09
Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍 LGTM

return "new_deployment" if ActiveRecord::Migrator.current_version.zero?
return "new_replica" if MiqServer.my_server.nil?
return "upgrade" if ActiveRecord::Migrator.needs_migration?
"redeployment"
Copy link
Member

Choose a reason for hiding this comment

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

amazing how much cleaner Ruby is compared to bash.

"new_deployment" => 0,
"new_replica" => 1,
"redeployment" => 2,
"upgrade" => 3
Copy link
Member

Choose a reason for hiding this comment

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

I think we should pick better numbers, but overall I like the approach: http://tldp.org/LDP/abs/html/exitcodes.html

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I honestly have no idea what those numbers mean 😕

Clearly, they're exit codes but from where? Can you put a link or a comment explaining them...

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 gave no thought to this at all.

The previous implementation was setting an environment variable to a string, but I figure an exit code was cleaner so I'm up for suggestions or I'll just go with 3, 4, 5, 6 (based on @Fryguy 's link)

@miq-bot
Copy link
Member

miq-bot commented Jun 20, 2017

Some comments on commits carbonin/manageiq@7ab13ea~...0b21880

lib/tasks/evm.rake

  • ⚠️ - 67 - Detected puts. Remove all debugging statements.

@miq-bot
Copy link
Member

miq-bot commented Jun 20, 2017

Checked commits carbonin/manageiq@7ab13ea~...0b21880 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍

@fbladilo
Copy link
Contributor

@carbonin This PR is beautiful 👍

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

❤️ I wish you linked to the shell code this replaces. The change is even better when you compare the complexity of the before/after code.

@jrafanie jrafanie merged commit 5d51aac into ManageIQ:master Jun 21, 2017
@jrafanie jrafanie added this to the Sprint 64 Ending Jul 3, 2017 milestone Jun 21, 2017
@carbonin carbonin deleted the add_deployment_status_rake_task branch October 13, 2017 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants