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

Issue #332 - Uninstalling all older gems when autoupdating #493

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

emilianodellacasa
Copy link
Contributor

In order to solve issue #332 I modified the code to execute a command to delete all older versions of Zold when auto updating

@0crat
Copy link
Collaborator

0crat commented Oct 15, 2018

Job #493 is now in scope, role is REV

@0crat
Copy link
Collaborator

0crat commented Oct 15, 2018

@yegor256/z everybody who has role REV is banned at #493; I won't be able to assign anyone automatically; consider assigning someone manually (as in §19), or invite more people (as in §51), or remove the job from the scope (as in §14)

@0crat
Copy link
Collaborator

0crat commented Oct 20, 2018

@yegor256/z everybody who has role REV is banned at #493; I won't be able to assign anyone automatically; consider assigning someone manually (as in §19), or invite more people (as in §51), or remove the job from the scope (as in §14)

@0crat
Copy link
Collaborator

0crat commented Oct 23, 2018

This pull request #493 is assigned to @SergeyKutsko/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @yegor256/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job

lib/zold/commands/node.rb Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Oct 24, 2018

Codecov Report

Merging #493 into master will decrease coverage by 0.31%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #493      +/-   ##
=========================================
- Coverage   33.42%   33.1%   -0.32%     
=========================================
  Files          63      62       -1     
  Lines        2941    2945       +4     
=========================================
- Hits          983     975       -8     
- Misses       1958    1970      +12
Impacted Files Coverage Δ
lib/zold/commands/node.rb 19.13% <0%> (-0.52%) ⬇️
lib/zold/wallet.rb 39.8% <0%> (-2.47%) ⬇️
lib/zold/txns.rb 52.63% <0%> (-2.37%) ⬇️
lib/zold/node/front.rb 34.72% <0%> (-2.1%) ⬇️
lib/zold/node/nodup_entrance.rb 30.3% <0%> (-2.05%) ⬇️
lib/zold/amount.rb 50% <0%> (-1.86%) ⬇️
lib/zold/commands/propagate.rb 28% <0%> (-1.79%) ⬇️
lib/zold/key.rb 37.83% <0%> (-1.64%) ⬇️
lib/zold/commands/diff.rb 35% <0%> (-1.59%) ⬇️
lib/zold/patch.rb 12% <0%> (-1.16%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6bc8a0...dba3c85. Read the comment docs.

@SergeyKutsko
Copy link
Contributor

@yegor256 I think it ready to go.

@0crat
Copy link
Collaborator

0crat commented Oct 28, 2018

@SergeyKutsko/z this job was assigned to you 5days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

@SergeyKutsko
Copy link
Contributor

SergeyKutsko commented Oct 28, 2018 via email

@@ -67,3 +67,11 @@ function halt_nodes {
done
}

function check_old_version_uninstalled {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@emilianodellacasa why this code is in the head if it's used only in one script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yegor256 Ok, removed the function

@yegor256
Copy link
Collaborator

@emilianodellacasa see above

@emilianodellacasa
Copy link
Contributor Author

@yegor256 All issues have been addressed

@@ -287,7 +287,9 @@ def nohup(opts)
begin
code = exec("#{myself} #{args.join(' ')}", nohup_log)
raise "Exit code is #{code}" if code != 0
exec(opts['nohup-command'], nohup_log)
Open3.popen3('gem uninstall zold -a --ignore-dependencies') do
exec(opts['nohup-command'], nohup_log)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@emilianodellacasa this looks a bit weird... First, we won't see the output of that uninstall. Second, what if the user wants to change the command and do sudo gem uninstall? It's impossible, right? I would better simply change the default command in nohup-command to something like gem uninstall && gem install

Copy link
Contributor Author

@emilianodellacasa emilianodellacasa Oct 30, 2018

Choose a reason for hiding this comment

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

@yegor256 What if we add another option to ./zold to change the uninstall command to whatever the user wants? Also, I don't see the problem in not showing the output of the uninstall, the user won't see it anyway.

Lastly, by including the uninstall command in the nohup-command as you propose , user could skip that part and I think it is important that there is only one instance of Zold gem running.

@yegor256
Copy link
Collaborator

@emilianodellacasa see above

@emilianodellacasa
Copy link
Contributor Author

@yegor256 see my comments above

@SergeyKutsko
Copy link
Contributor

@emilianodellacasa @yegor256 So what we need update to get this task merged?

@emilianodellacasa
Copy link
Contributor Author

@yegor256 When I uninstall zold gem, shall I also uninstall zold-score.

Does zold-score auto-updates as well together with zold?

@emilianodellacasa
Copy link
Contributor Author

@SergeyKutsko I am waiting for some comments from @yegor256

@yegor256
Copy link
Collaborator

@emilianodellacasa I'm not sure. How it works in Ruby?

@emilianodellacasa
Copy link
Contributor Author

@yegor256 Actually, zold-score should auto update as well, so it will be better to uninstall that gem as well before reinstalling zold (and, consequently, zold-score)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants