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

Why rultor use .rultor.yml from base branch and not use it from branch inside pull request #1184

Open
Slach opened this issue Jun 2, 2017 · 11 comments
Labels

Comments

@Slach
Copy link

Slach commented Jun 2, 2017

see Slach/test_tasks#5 (comment)
i added sudo to my rultor.yml
but when rulton run install
he used old instruction without sudo
why?

@0crat
Copy link
Collaborator

0crat commented Jun 2, 2017

@yegor256 please, pay attention to this issue

@driver733
Copy link

driver733 commented Jun 2, 2017

@Slach rultor uses .rultor.yml only from master branch, so in order to get rultor to behave differently you have to manually commit and push a new (corrected) version of .rultor.yml file to the master branch.
@yegor256 I think this should be mentioned in the rultor's guidelines/instructions. I faced this issue myself and solved it only by guessing that rultor always uses the .rultor.yml file from the master branch.

@yegor256
Copy link
Owner

yegor256 commented Jun 9, 2017

@Slach this is how it should work, according to #1039 But it's not fully implemented, as far as I can tell.

@yegor256 yegor256 closed this as completed Jun 9, 2017
@yegor256 yegor256 reopened this Jun 9, 2017
@0crat
Copy link
Collaborator

0crat commented Jun 9, 2017

@yegor256 please, pay attention to this issue

@yegor256
Copy link
Owner

yegor256 commented Jun 9, 2017

@Slach this code doesn't do anything, but it should actually fetch and merge: https://github.com/yegor256/rultor/blob/1.64.1/src/main/java/com/rultor/profiles/Profiles.java#L96-L99

@extsoft
Copy link

extsoft commented Jun 30, 2019

I have a similar behavior for Rultor. I mean the Rultor follows masters .rultor.yml and ignores changes from the forked branch (see bees-hive/elegant-git#136 (comment); the testing is failed as a new version of docker image is not used). @yegor256 @driver733 do I need to create an additional issue about it (this issue is silent for 2 years)?

@dgroup
Copy link

dgroup commented Jun 14, 2020

@yegor256 there might be security issue here, as someone in pull request may add

script: |-
   ...
   env
   ...
   cat your.assets 

and your variables or assets will be available in logs...

Be very careful with such type of feature requests...

@driver733
Copy link

@extsoft As pointed out by @dgroup, I think that Rultor should use only the master branch config.

@extsoft
Copy link

extsoft commented Jun 23, 2020

@driver733 I understand the security risks and they should be handled as well.

However, from a functional point of view, I update the branch's config with an incompatible change. And the behavior I described should happen only in the branches that have the new changes, but all other branches should behave in the correspondence of their own config - it's like other CI servers work.

Now, If I push an incompatible change to master, all other branches will fail until they pull master's changes.

And I can't submit a task that updates the Rultor config and does some changes in one pull request. I must split. But this makes review procedure more complex.

Again. I understand that there may be a security issue. But this is a non-functional requirement, and it should be handled properly. And this issue is about a feature request. So, security requirements should be a part of the feature implementation as well as other non-functional requirements.

@dgroup @driver733 does it make sense?

@driver733
Copy link

@extsoft You're right, it makes sense.

I am using Github Actions and it works the following way:

  1. CI uses config in the according branch
  2. Repository forks are not permitted to access secrets.

I think this kind of behavior is the most preferable.

@pnatashap
Copy link
Contributor

pnatashap commented Feb 18, 2024

@driver733 @yegor256 now it is by design feature. may be this should be added to documentation? there are many questions about it. And now fail can not be changed at all. #1459

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

7 participants