Skip to content

Conversation

@RSickenberg
Copy link
Contributor

@RSickenberg RSickenberg commented Mar 30, 2022

Allow me to fix this:
#170

Note I couldn't run the PHPUnit in a clean env like the CI.

Greetings to everyone in Lausanne 👋🏼 💚

@RSickenberg RSickenberg marked this pull request as ready for review March 30, 2022 12:55
@jeanmonod
Copy link
Member

Hi @RSickenberg,

Thanks a lot for this PR 👍

There is still an issue with the tests because they rely on a small embedded Git repo: https://github.com/liip/RMT/blob/master/test/Liip/RMT/Tests/Unit/VCS/gitRepo.zip where the branch is still named master.

This is showing us, that changing the main branch name is a breaking change for existing RMT install (existing projects that are still using the master naming). We should find a way to make this configurable on the config. So we can ask users to add this config into rmt.yml if there are in this situation. Do you want to try to do this?

I imagine something like this:

vcs: 
  name: git
  main-branch: master

To locally run the tests, there is a documentation here: https://github.com/liip/RMT/blob/master/README.md#tests

Thanks again for your efforts!

@RSickenberg
Copy link
Contributor Author

Thanks, @jeanmonod for the input! Like I wrote on cd3b975, I didn't know about this small repo before so this answers the question I had about this.

I indeed followed the documentation, but my tests are differing from the CI, I have ~6/7 failures VS 2 (one should be fixed now) on the CI.
The issue must be due to the fact I'm running this on my machine with PHP 8.1.4.

I would love to contribute more on a migration script, let me some time to do so 🙂

@jeanmonod
Copy link
Member

@RSickenberg Changing the repo is an option to fix the tests. But you are still leaving alone all the users working with a master branch. For backward compatibility reason, we need a way to configure the main branch name in the config.
If you can do it, that's great, otherwise I will try to find someone on Lausanne to do it :)

@RSickenberg
Copy link
Contributor Author

@jeanmonod Sure, I will find a way to take care of that!

@RSickenberg
Copy link
Contributor Author

Hello @jeanmonod, could you run the tests?

What do you think of the current approach?

Copy link
Member

@jeanmonod jeanmonod left a comment

Choose a reason for hiding this comment

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

Thanks a lot @RSickenberg

I just did a first review! At this end, it's not an easy-pick at all. Sorry for that.

Can I ask you one more thing? To keep master branch name into the functional tests. Like this: 1) It's testing to use a different branch than main. So it's ensuring backward compat 2) We don't need to change the embedded Git repo?

$this->initGit();
$this->createConfig('simple', 'vcs-tag', ['vcs' => 'git']);
exec('git branch --show-current', $branch);
self::assertEquals('main', $branch[0]);
Copy link
Member

Choose a reason for hiding this comment

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

What are you testing here? To me this is not testing RMT...

@RSickenberg RSickenberg requested a review from jeanmonod April 21, 2022 07:54
@RSickenberg
Copy link
Contributor Author

Hey @jeanmonod, can you run the tests, please? I saw I checked your comments so you might want to look at it :)

Regards

@jeanmonod
Copy link
Member

Hi @RSickenberg,
Sorry for my long silence, but I was off for the past 15 days. I long but very resourcing break :)
I will take a look at your lasted commit as soon as possible

@RSickenberg
Copy link
Contributor Author

Hello @jeanmonod, no worries! Hope this was a good break 🤗 !

Sure, let me know if you need anything.

@dbu
Copy link
Member

dbu commented Nov 29, 2025

any chance we can wrap this up? the documentation is still speaking of master, when most projects are using main nowadays.

Co-authored-by: Romain Sickenberg <haux49@gmail.com>
@dbu
Copy link
Member

dbu commented Dec 3, 2025

i fixed a regression and merged this in #186

@dbu dbu closed this Dec 3, 2025
@dbu
Copy link
Member

dbu commented Dec 3, 2025

thanks a lot for the work @RSickenberg

@RSickenberg
Copy link
Contributor Author

Anytime @dbu, thanks for the follow-up. This lasted for a long time :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants