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

Deprecate Using the yegor256/rultor Docker Image for Other Projects #1018

Closed
original-brownbear opened this issue Feb 22, 2016 · 49 comments
Closed
Labels

Comments

@original-brownbear
Copy link
Contributor

original-brownbear commented Feb 22, 2016

Currently we have many of our own and I suspect also other projects running on the default Docker image for Rultor.

This is a significant risk to the stability of their builds.

Problem

Now what these projects have is this for their install section in the config file (or similar) this is from Netbout:

  sudo gem install --no-ri --no-rdoc pdd
  sudo gem install --no-ri --no-rdoc est

For Rultor (and as a result those other projects) this means that Rultor has to:

  1. Provide the sudo package/command and not stop doing so whenever a change to the Dockerfile is made, even if Rultor itself does not need the sudo command.
  2. Not install any gems to the global scope that interfere with pdd or est

While so far this only broke Gem related things, using the default image in other projects introduces risks elsewhere too, say in using the Rultor supplied Maven. For example we upgraded it to 3.3.9 from 3.2.1 in
#1005.
In this case I know of no backward compatibility problems (except for breaking Java 6 projects using Maven), but what if the next time Rultor needs an upgrade to its Maven version and it is not compatible with what e.g. Netbout needs?

Fix Has to Provide:

  1. A clear notice at the very end of the build output when building anything but Rultor itself while using the default yegor256/rultor image ( not providing your own ):
  2. A short explanation:
    • clearly warning about the dangers of using Rultor without supplying your own Docker image/Dockerfile and hence falling back to yegor256/rultor
    • stating that running Rultor against the default image is deprecated.

@original-brownbear
Copy link
Contributor Author

@alex-palevsky this is valid.

@original-brownbear
Copy link
Contributor Author

@alex-palevsky this is a bug.

@alex-palevsky
Copy link
Contributor

@alex-palevsky this is a bug.

@original-brownbear tag "bug" added

@alex-palevsky
Copy link
Contributor

@original-brownbear I set milestone here to 2.0, let me know if it is wrong

@alex-palevsky
Copy link
Contributor

@original-brownbear many thanks for the report, I topped your account for 30 mins, transaction AP-3E950352BE375454W

@original-brownbear
Copy link
Contributor Author

@alex-palevsky this is urgent.

@alex-palevsky
Copy link
Contributor

@alex-palevsky this is urgent.

@original-brownbear right, I added "urgent" label

@alex-palevsky
Copy link
Contributor

@gumbelmj this ticket is yours now, please proceed, and keep in mind this. Any technical questions you should ask right here; Task's budget is 30 mins (see this for explanation)

@gumbelmj
Copy link
Contributor

gumbelmj commented Mar 4, 2016

@alex-palevsky please assign to someone else

@alex-palevsky
Copy link
Contributor

@alex-palevsky please assign to someone else

@gumbelmj I deducted 30 points from your rating

@alex-palevsky
Copy link
Contributor

@alex-palevsky please assign to someone else

@gumbelmj no problem, I will try to find somebody else

@alex-palevsky
Copy link
Contributor

@sebing help us, the task is yours

@sebing
Copy link

sebing commented Mar 4, 2016

@original-brownbear Can you give some clarity in the work to be done?

  • What to make multiline?
  • Should I modify com.rultor.agents.docker.DockerExec to throw an exception if it is the default image(i have to figure it out what the mean myself)?

@original-brownbear
Copy link
Contributor Author

@sebing sure, let my try to give a proper answer here :)

What to make multiline?
Just the notice at the bottom of a Rultor build, like say we look at this example log:

+ mv /home/r/repo .
++ whoami
+ chown -R root repo
+ '[' -n '' ']'
++ whoami
+ sudo chown -R rultor repo
+ cd repo
+ git push origin master
To git@github.com:someuser/somerepo.git
   af0fe22..e506072  master -> master
container 7d32415742c0fb09f4a5d87bd943c6725243c2c3256ed2139c56e3c21c1a9160 is dead
Fri Mar  4 12:18:13 UTC 2016

and assume the user had either no image or the yegor256/rultor image specified for his build in the .rultor.yml. Then the log should get a multi-line note appended, telling him that this is a "deprecated thing" to do ... along the lines of:

+ mv /home/r/repo .
++ whoami
+ chown -R root repo
+ '[' -n '' ']'
++ whoami
+ sudo chown -R rultor repo
+ cd repo
+ git push origin master
To git@github.com:someuser/somerepo.git
   af0fe22..e506072  master -> master
container 7d32415742c0fb09f4a5d87bd943c6725243c2c3256ed2139c56e3c21c1a9160 is dead
Fri Mar  4 12:18:13 UTC 2016
#### Deprecation Notice ####
You are using the Rultor default Docker image in your build.
.... some short version of above explanation as to why this is bad ....
########################

Should I modify com.rultor.agents.docker.DockerExec to throw an exception if it is the default image(i have to figure it out what the mean myself)?

No that class is not where you can do this, it is in fact only used to handle the cleaning of old images (and currently not really doing anything ... there's a ticket for that though.)
I suggested starting at com.rultor.agents.daemons.StartsDaemon#run, this function is what actually triggers the Docker run.
Also take a look at com/rultor/agents/req/_head.sh which is what eventually gets executed by the above function.
Lastly, this is not something that needs an exception. There is no error here that needs to be handled, it's just a user input issue that needs a simply but clear answer for the user :)

Let me know if this is enough to get you started here :)

@sebing
Copy link

sebing commented Mar 4, 2016

@original-brownbear
Thank you for the help. I think, I have a better understanding now.

  • For the Deprecation Notice
    Should I hardcode the details in com/rulto/agents/daemons/end.sh? And assuming {image} variable is set?

-Deprecating the default image.
com/rultor/agents/req/_head.sh If the {image} variable is yegor256/rultor i will echo a waring message?

@original-brownbear
Copy link
Contributor Author

@sebing I think it would be best to put the logic for adding the notice into the Java code and just using and actually printing it from there. If we put it in bash it's a lot trickier to add a proper test for this.

But yes logically every time yegor256/rultor is used echo the warning, except for when you're building the actual https://github.com/yegor256/rultor/ repo of cause :)
PS: sorry for closing/reopen ... missclick :)

@sebing
Copy link

sebing commented Mar 4, 2016

@original-brownbear I don't understand. The logger output is not in the output of rultor log

@sebing
Copy link

sebing commented Mar 7, 2016

@original-brownbear Thanks for the information, I will keep them in mind.

@sebing
Copy link

sebing commented Mar 7, 2016

@alex-palevsky I have made a PR(#1045)

@sebing
Copy link

sebing commented Mar 7, 2016

@alex-palevsky I believe my solution to this issue is not up to the acceptable quality.

@sebing
Copy link

sebing commented Mar 9, 2016

@alex-palevsky As I am taking too much time to understand the code without any documentation. May be you can assign the task to someone else?

@alex-palevsky
Copy link
Contributor

@alex-palevsky As I am taking too much time to understand the code without any documentation. May be you can assign the task to someone else?

@sebing -30 points to your rating :(

@alex-palevsky
Copy link
Contributor

@alex-palevsky As I am taking too much time to understand the code without any documentation. May be you can assign the task to someone else?

@sebing right, I'll try to find someone else for this task

@sebing
Copy link

sebing commented Mar 11, 2016

@alex-palevsky The task is complete. Waiting for @original-brownbear to merge.
I messaged you 2 days ago as I have great difficulty in understanding the code. But i managed to finish it yesterday.

@sebing
Copy link

sebing commented Mar 11, 2016

@alex-palevsky as the the PR(#1045) is merged. Can you close the issue?

@original-brownbear
Copy link
Contributor Author

closing :) thanks!

@sebing
Copy link

sebing commented Mar 11, 2016

@alex-palevsky So this will be considered as my work? ;)

@alex-palevsky
Copy link
Contributor

@original-brownbear 2 puzzles were created here: 1018-15a5404e, 1018-b83a728c/#1052

@sebing
Copy link

sebing commented Mar 11, 2016

@alex-palevsky As I finished the task, Did I get the point you have minus previously? Or that is the punishment for telling you that the code is difficult to understand ;)..

@sebing
Copy link

sebing commented Mar 14, 2016

@alex-palevsky As this issue is closed and deployed. When will the rating will be updated? or I will end up with -30 :(

@alex-palevsky
Copy link
Contributor

@alex-palevsky As this issue is closed and deployed. When will the rating will be updated? or I will end up with -30 :(

@sebing no, this task is not yours that's why no points and no payments to you (if you disagree, talk to project architect)

@sebing
Copy link

sebing commented Mar 17, 2016

@original-brownbear As I have finished this task in time. Do you think it is fair

  • not get paid
  • and get a -30 points?

@original-brownbear
Copy link
Contributor Author

@sebing I'm sorry but fair is not the category here in the first place. @alex-palevsky 's actions are in full compliance with the our policy. You asked to be reassigned, he reassigned you.
The fact that there was some delay here is pretty much irrelevant.

Still, regarding fair:
I took a little time looking into this and found yegor256/takes#600 and associated pull request yegor256/takes#632.
As you can see this is not the first time the exact situation happened for you. In that ticket you also put in a PR after a ticket was reassigned already effectively ( you put it in after you 48h deadline! ).
But still the reviewer got paid there, even though his work did not at all contribute to the project,
given that the PR then needed to be closed since the ticket was reassigned.
=> Please understand this, the only way the way we work here is feasible is, if rules are strictly enforced. If spots like this one are not enforced, then the Architects and PMs in projects need to spend a lot of additional time working through situations like this one.
Since no one is paid for handling situations like the one at hand in a personal manner, we simply cannot have them. Me having answered this in detail was purely on my own time, so was you working on an issue you already resigned from.
I'm sorry, but this situation is in no way open to interpretation I think, please try to follow the rules more closely in the future.

@sebing
Copy link

sebing commented Mar 17, 2016

@original-brownbear Thanks for the detailed reply it is great to know the detail always from you :).

-As you bring the issue (yegor256/takes#600). I agree 100% it was my mistake. I can clarify my misunderstanding. @alice messaged me I have to do only 2 tasks {or that is what I have understood :)} So I kept the tasks in the pipeline and forget about it. As there is no direction, for the new joined team member. It is a bit difficult to know how we work here..(I don't think, my clarification is forgivable as it is chasing me all the task i do)

@sebing
Copy link

sebing commented Mar 17, 2016

@original-brownbear @alex-palevsky Lesson learned here. Never say no. :)

@original-brownbear
Copy link
Contributor Author

@sebing the lesson simply is, follow the rules without any exceptions ever.
You had two options here:

  1. Asking for more time.
  2. Saying no.
    You said no (Deprecate Using the yegor256/rultor Docker Image for Other Projects #1018 (comment))
    What you're doing when asking to give you a break here, is simply asking others to spent unpaid time for you, in order to sort these things out. If this would be done here, it would not be "fair" to not do this for everyone else and that would render rules pointless.

The only reason I brought the other mistake you made in Takes up, was to illustrate that these things cut both ways, it was not in any way relevant even that you made the mistake there (In fact I simply know about that one because I was waiting for the CR in there and was "stalking" him at the time ...).

... in the sense of http://www.yegor256.com/2014/04/20/how-hourly-rate-is-calculated.html#lengthy-discussions => just don't do it again, this issue is closed sorry.

@sebing
Copy link

sebing commented Mar 17, 2016

@original-brownbear Okay i will keep quiet :) I won't be over-smart and assess the urgency of the issue anymore ;)
So you fixed the issue without paying me, I understand it was my mistake to underestimate my ability that i can fix the issue.

Thanks for the explanations.

@sebing
Copy link

sebing commented Mar 17, 2016

@alex-palevsky @original-brownbear Just for the record, If anyone looking at the the treads. To know that you guys are co-coordinately working in "fairly"

  • @alex-palevsky remove me from the issue after my request to remove for two days on Mar 11, 2016, 1:52 PM GMT
  • The merge request is made in the pr at Mar 11, 2016, 12:05 PM GMT

So the manager knows theoretically the work is done and if I am removed from the task, I won't be needed to be paid.

@alex-palevsky
Copy link
Contributor

@alex-palevsky @original-brownbear Just for the record, If anyone looking at the the treads. To know that you guys are co-coordinately working in "fairly"

  • @alex-palevsky remove me from the issue after my request to remove for two days on Mar 11, 2016, 1:52 PM GMT
  • The merge request is made in the pr at Mar 11, 2016, 12:05 PM GMT

So the manager knows theoretically the work is done and if I am removed from the task, I won't be needed to be paid.

@sebing again, sorry about this

@alex-palevsky
Copy link
Contributor

@original-brownbear the last puzzle 1053-dc86a5cc/#1069 originated from here solved

@0pdd
Copy link
Collaborator

0pdd commented Feb 14, 2024

@original-brownbear all 3 puzzles are solved here: #1053, #1161, #unknown.

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

No branches or pull requests

5 participants