Rultor Uses the Target Branch's .rultor.yml on Merges

[original-brownbear]

Currently we have a situation where pull requests like e.g. #1038 can get stuck in a manner Rultor users cannot resolve without manually intervening in their master branch.

The problem is this:

We are currently reading the .rultor.yml from the master branch, not the fork branch.
This means that whatever changes a user makes to the .rultor.yml will not affect the run to merge those changes, but only the next run.
Reading the .rultor.yml from master is obviously correct for authentication purposes, but it is also obviously wrong for the dependency parts ( merge, install, docker etc.) parts of the .rultor.yml.
Same as with a .travis.yml these parts need to come from the same branch as the source code, otherwise any change in dependencies becomes impossible when strictly allowing only Rultor on the master branch.
This is especially bad since the opposite is also possible, the merge with changes in the .rultor.yml could work, even when those changes break the build. Hence merging a broken .rultor.yml would cause development to become stuck in any case.

Example

You want to upgrade your project from Java 7 to Java 8 and change the image: section in the .rultor.yml from JDK7 to JDK8, as well as some code in a way that is not Java 7 compatible anymore.
Merging this is impossible, because the run will still use the JDK7 from master. Only after the merge would JDK8 be used for subsequent merges.

Fix must include:

• Reading .rultor.yml from the merge result when building, not from master
• Reading .rultor.yml for authentication purposes from master still
  • Authentication purposes being defined as the lists for commanders and architects

---

• 1039-d8a49c64/Profiles.java:108-115: Add integration test to be... #1072 (by )

[original-brownbear]

@alex-palevsky this is a bug.

[alex-palevsky]

@alex-palevsky this is a bug.

@original-brownbear I tagged this as "bug"

[alex-palevsky]

@original-brownbear since the ticket has no milestone I set it to 2.0

[alex-palevsky]

@original-brownbear many thanks, 30 mins added to your acc for reporting this bug, pmt ID AP-6YF16319TE2785433

[original-brownbear]

@alex-palevsky this is urgent.

[alex-palevsky]

@alex-palevsky this is urgent.

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

[alex-palevsky]

@vkuchyn you may proceed, it's yours

[vkuchyn]

@original-brownbear I have a question:
authentication purposes listed in section assets. Am I correct?

[original-brownbear]

@vkuchyn no, check last line in the issue description, made it clearer :)

[vkuchyn]

@original-brownbear thanks. I have another questions about implementation.
As I understood design - class Routine triggers active talks (I suggest, this is comments to rultor collaborator in PR), creates profile based on it and run chain of agents to implement rultor logic. Correct me if I am wrong.
On the step of profile creation (see source file) if talk type is not merge - we create profile based on fork. If it is so - first clause of issue is already done.
About second - authentication purposes. We need somehow merge information about yaml in GithubProfile class, the only place where we can do it - on the stage of fetching it in Profiles and exactly for merge command first create Profile on master branch, then create Profile based on fork branch and then somehow merge them according to current issue requirements. What I see - current design is not supporting new requirements and we need to change it.
The questions are:

1. Am I correct in my conclusions?
2. If so - what is your recommendations about design changes?

[original-brownbear]

@vkuchyn phew :) tricky questions, but actually I think you got pretty far with this already, let me say this:
we create profile based on fork. If it is so - first clause of issue is already done.

=> Take a good look at the linked PR here: https://github.com/yegor256/rultor/pull/1038/files
... evidently this is not working :), seems to me this is likely a very trivial and easy to reproduce via a unit-test issue given the code you linked. Clearly that code is supposed to do what I'm asking for here, but it doesn't do that as can be seen in that PR ( I removed the instructions from the install section that called the broken jekyll command, still it was called by Rultor ).

What I see - current design is not supporting new requirements and we need to change it.
Keep the answer to the first question in mind here :)
a. I think right now we're simply only using all the info from master when interpreting the .rultor.yml, also we already do have (apparently broken) code for handling the merge result for the profile. I think if you figure out the first part (using the fork result), which appears to me is a reasonable thing in the scope of this ticket, likely the second part will be trivial and automatic for you.
But if this turns out to not be the case => http://www.yegor256.com/2009/03/04/pdd.html :),

[vkuchyn]

@original-brownbear seems, I found a place where the problem is.
I have 2 possible solutions:

1. Fix it in StartsRequest.java
2. Fix it in Profile.Defaults

What do you think is better?

UPD: The problem is in DockerRun.scripts() which takes profile as a parameter. But profile comes from Profiles.fetch I wrote above. So, as for me according to code - for merge command should be forked yml used. Of course, I see log output of problem issue where this happened and have no idea why it so. Fo diagnostic I need some integration test but I didn't find proper one.
So, now I've stucked and require some help.
Problems are:

1. I still see that code should work correctly
2. I have no idea how to localize problem with unit test

[original-brownbear]

@vkuchyn you have to admit you're giving me very little to go by in making the choice between one and two :)
Fortunately, the choice is yours ;) Really I mean, simply pick the one that you'll have the easier time writing a proper test reproducing the issue for. You know, once you have that failing test, you also know for sure whether or not you found the place where the issue is :)

[vkuchyn]

@original-brownbear thanks for quick reaction. I didn't expect such speed ) Please, see my comment above - I updated it

[original-brownbear]

Fo diagnostic I need some integration test but I didn't find proper one.

Well, if you can't find one, make one :) Again, keep those puzzles in mind, don't try to do anything big here. Just add something + Puzzle(s) that move us towards the solution if you think this requires a very tricky integration test to reproduce.

I still see that code should work correctly
Do we have unit-tests describing the behavior too, or does it just "look ok" superficially ? :)

[vkuchyn]

@original-brownbear we don't have unit tests describing the behavior. It is only "looks ok" superficially.

[vkuchyn]

@original-brownbear I'd like to skip this issue according to no-obligation principle.

[original-brownbear]

@vkuchyn sure no problem, I'm the wrong person to talk to about that though.
You gotta ping @alex-palevsky for that and ask him to assign this ticket to someone else.

[vkuchyn]

@alex-palevsky I'd like to skip this issue according to no-obligation principle. Please, find some one else for this issue

[alex-palevsky]

@alex-palevsky I'd like to skip this issue according to no-obligation principle. Please, find some one else for this issue

@vkuchyn -30 points to your rating

[alex-palevsky]

@alex-palevsky I'd like to skip this issue according to no-obligation principle. Please, find some one else for this issue

@vkuchyn OK, I will try to assign someone else

[alex-palevsky]

@xupyprmv this is your task,please go ahead

[original-brownbear]

@xupyprmv any progress here so far ?

[xupyprmv]

@original-brownbear I didn't start yet. But issue looks clear. I will try to resolve it.

[xupyprmv]

@alex-palevsky I need more time here, please.

[alex-palevsky]

@alex-palevsky I need more time here, please.

@xupyprmv right, take your time and thanks for informing

[xupyprmv]

@alex-palevsky PR #1070 was merged. This ticket is closed.

[alex-palevsky]

@alex-palevsky PR #1070 was merged. This ticket is closed.

@xupyprmv thank you!

[alex-palevsky]

@original-brownbear once 1039-d8a49c64/#1072 puzzle is resolved (later, in another ticket), this ticket will be fully complete

[alex-palevsky]

@xupyprmv Thank you, I have added 30 mins to your account in payment/transaction "AP-8K392850DC1718548", time consumed: 334 hours and 36 mins; +30 added to your rating, at the moment it is: +300

[alex-palevsky]

@original-brownbear last puzzled solved 1039-d8a49c64/#1072