-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
#6401 Support to "alwaysSignOff" commits #6402
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JonasHelming please take a look at the failing CI.
The error comes from a missing binding during the test:
theia/packages/git/src/browser/git-repository-provider.spec.ts
Lines 93 to 95 in e0074c5
testContainer = new Container(); | |
testContainer.bind(GitRepositoryProvider).toSelf().inSingletonScope(); | |
testContainer.bind(Git).toConstantValue(mockGit); |
Adding the binding for GitPreferences
worked for me locally:
...
testContainer.bind(GitPreferences).toConstantValue(<GitPreferences>{});
...
Hi @JonasHelming! While this seems to work as advertised, I think it's confusing that Why not aligning with vscode and implementing a general |
Hey, thanks for the feedback, I will have a look at the failing test cases.
Maybe a potential solution is to show in the commit message text field "Message (Ctrl+Enter to commit (signed off) on {$branch}? Another potential solution is to introduce both preferences. Finally, if you dislike the "CTRL+ENTER" only approach, I can rework it to match VS Code exactly although as mentioned, I find that more confusing. What do you think? |
It would be good to be aligned with VS Code, especially we are going to drop native extension eventually in favor of VS Code one. |
…onas Helming <jhelming@eclipsesource.com> Signed-off-by: Jonas Helming <jhelming@eclipsesource.com>
I have no idea, why the build on Mac fails ... |
restarted the job, was too many requests to GitHub to download vscode-rigrep |
It does not help :( is it because a fork don't get our github tokens? cc @marcdumais-work ? |
The same happens in this PR :( #6480 |
Yes, exactly this https://docs.travis-ci.com/user/environment-variables/#defining-variables-in-repository-settings |
ok, so then we can ignore the build failure and merge? |
In case of doubt we can restart the failing job - it will pass eventually. |
I've manually restarted the build, the CI is now all green :) |
@AlexTugarev @kittaakos Could you finish the review please? You had some comments above. Were they addressed? If so, please approve and merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good and it works as expected. It is good to merge 👍
Thank you for your help, @JonasHelming!
I have verified the changeset in Gitpod by manually adjusting both .vscode/settings.json
and .theia/settings.json
files, committing to the local repo, and checking the git log
.
Signed-off-by: Jonas Helming jhelming@eclipsesource.com
What it does
How to test
turn the preference on (under Git) and commit via CTRL+ENTER. Check the the commit is signed off
Review checklist
Reminder for reviewers