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.

Sign up for GitHub

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

Jump to bottom

[ADD] rebase_bot #71

Merged
merged 2 commits into from
Aug 7, 2021
Merged

[ADD] rebase_bot #71

merged 2 commits into from
Aug 7, 2021

Conversation

MiquelRForgeFlow
Copy link
Contributor

New command. I hope it will be useful.

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the master-add-rebase_bot branch 4 times, most recently from f4e3f9e to da58f32 Compare September 4, 2019 15:29
@MiquelRForgeFlow
Copy link
Contributor Author

@sbidoul ready to be reviewed! :)

sbidoul
sbidoul reviewed Sep 4, 2019
View reviewed changes
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

I've mixed feeling about this. Mainly because force pushing to a contributor branch... well that sounds intrusive to me.

requirements.txt Outdated Show resolved Hide resolved
src/oca_github_bot/config.py Outdated Show resolved Hide resolved
src/oca_github_bot/tasks/rebase_bot.py Outdated Show resolved Hide resolved
src/oca_github_bot/tasks/rebase_bot.py Outdated Show resolved Hide resolved
src/oca_github_bot/tasks/rebase_bot.py Outdated Show resolved Hide resolved
@sbidoul sbidoul added Bot Task A task the bot does or should do enhancement New feature or request labels Sep 4, 2019
@MiquelRForgeFlow MiquelRForgeFlow force-pushed the master-add-rebase_bot branch 2 times, most recently from 906badf to bba97d1 Compare September 4, 2019 17:52
@MiquelRForgeFlow
Copy link
Contributor Author

MiquelRForgeFlow commented Sep 4, 2019 •
edited
Loading

@sbidoul all comments attended.

In my opinion, I think it's not intrusive. This is just a tool. It depends of the maintainers on how they use the tool. If they are rebasing continuously around wantonly, then they may be in the intrusive side. But I think the correct use of this tool is to be used sporadically, when the creator of the PR is not rebasing or don't know how to rebase correctly. Or simply, the creator of the PR wants to rebase (but don't want to push anymore) so they call this tool :)

LoisRForgeFlow
LoisRForgeFlow reviewed Sep 5, 2019
View reviewed changes
.isort.cfg Outdated Show resolved Hide resolved
@MiquelRForgeFlow
Copy link
Contributor Author

@sbidoul all comments attended :)

sbidoul
sbidoul requested changes Sep 5, 2019
View reviewed changes
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

You need to adress this comment #71 (comment). BeautifulSoup is not the right approach for sure.

Also I was expecting to have some call to temporary_clone? Where is the part where you git clone the repo to work on?

Does the git push to https without token work?

There must still be some work to do to finalize this.

I'll check back end of September.

@MiquelRForgeFlow
Copy link
Contributor Author

MiquelRForgeFlow commented Sep 5, 2019 •
edited
Loading

Why the clone? I don't understand. You fetch the branch, does the rebase on local and then push again to the same branch. It's a simple process, I don't see why adding more complicated steps. :S Well, I may be wrong, not sure of all of this kind of process.

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the master-add-rebase_bot branch 2 times, most recently from fcf38d6 to 9216d15 Compare September 5, 2019 18:36
@MiquelRForgeFlow
Copy link
Contributor Author

Done now without using beautifulsoup 🎉🎉🎉🎉🎉🎉

@pedrobaeza pedrobaeza mentioned this pull request Sep 11, 2019
Merged
1 task
@MiquelRForgeFlow MiquelRForgeFlow force-pushed the master-add-rebase_bot branch 2 times, most recently from 3b73602 to a10170a Compare September 25, 2019 12:27
@MiquelRForgeFlow
Copy link
Contributor Author

@sbidoul I have added the call to temporary_clone. What do you think now?

@sbidoul
Copy link
Member

sbidoul commented Sep 28, 2019

It looks better. Travis is red though. Generally I'm concerned that you may not have done enough manual tests on this PR. I'll be around at #OCAdays to help you running the bot locally for testing.

legalsylvain
legalsylvain approved these changes Mar 26, 2020
View reviewed changes
Copy link
Collaborator

@legalsylvain legalsylvain 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 single question. LGTM otherwise.

code review / no test.

src/oca_github_bot/manifest.py Outdated Show resolved Hide resolved
sbidoul
sbidoul requested changes Mar 28, 2020
View reviewed changes
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

@MiquelRForgeFlow a few minor comments and a some more significant one in the main function.

tests/test_commands.py Show resolved Hide resolved
src/oca_github_bot/manifest.py Outdated Show resolved Hide resolved
src/oca_github_bot/manifest.py Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
src/oca_github_bot/tasks/rebase_bot.py Outdated Show resolved Hide resolved
src/oca_github_bot/tasks/rebase_bot.py Outdated Show resolved Hide resolved
src/oca_github_bot/tasks/rebase_bot.py Outdated Show resolved Hide resolved
src/oca_github_bot/tasks/rebase_bot.py Outdated Show resolved Hide resolved
@sbidoul sbidoul mentioned this pull request Mar 28, 2020
Merged
@legalsylvain legalsylvain mentioned this pull request Mar 29, 2020
Open
@MiquelRForgeFlow MiquelRForgeFlow force-pushed the master-add-rebase_bot branch 2 times, most recently from aca3627 to 53b5ba2 Compare April 22, 2020 18:31
@MiquelRForgeFlow
Copy link
Contributor Author

@sbidoul Thanks for your your tips. I think now it works. Please, can you check again?

@legalsylvain
Copy link
Collaborator

@MiquelRForgeFlow could you fix conflict ?

@bealdav
Copy link
Member

bealdav commented Aug 2, 2021

l love the idea of this PR. Any hope to be merged in September ?

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the master-add-rebase_bot branch 2 times, most recently from d05a0e3 to 2dcacf9 Compare August 2, 2021 10:21
@MiquelRForgeFlow
Copy link
Contributor Author

@legalsylvain done

legalsylvain
legalsylvain reviewed Aug 2, 2021
View reviewed changes
Copy link
Collaborator

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

I just deployed this PR for my GRAP bot.

Tested to rebase here : grap-org-test-bot/github-ocabot-test#15

It doesn't work. maybe I missed something in the deployment, but the code seems up to date.

Another people are using this branch on production ?

legalsylvain
legalsylvain requested changes Aug 2, 2021
View reviewed changes
Copy link
Collaborator

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

The OCABOT_USAGE should be updated.

https://github.com/OCA/oca-github-bot/blob/master/src/oca_github_bot/config.py#L110

@bealdav
Copy link
Member

bealdav commented Aug 2, 2021

I don't know if it's in the intention of this PR but it could be a good option if author of the PR could apply itself this rebase cmd. But not mandatory

@MiquelRForgeFlow
Copy link
Contributor Author

@bealdav yes, author of PR can rebase because they can push.

@MiquelRForgeFlow
[REF] _user_can_merge -> _user_can_push
3cd3b28 
Renamed and relocated method to be able to be used in other bots.
@MiquelRForgeFlow
[ADD] rebase_bot
489b4a1
legalsylvain
legalsylvain approved these changes Aug 6, 2021
View reviewed changes
Copy link
Collaborator

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Hi.

I justed deployed the last version of this PR that is fixing authentication trouble.

Tested on a demo repository, on that PR : grap-org-test-bot/github-ocabot-test#15

Works like a charm. Thanks a lot for this great addition @MiquelRForgeFlow !

image

functional test / No code review.

@MiquelRForgeFlow
Copy link
Contributor Author

@sbidoul now that it has been tested, I hope we can proceed forward :)

@sbidoul
Copy link
Member

sbidoul commented Aug 6, 2021

Great. I'll now re-read since it touches a security sensitive part.

@sbidoul
Copy link
Member

sbidoul commented Aug 7, 2021

Thanks for testing @legalsylvain
Looks good, now, thanks !

@sbidoul sbidoul merged commit a1c656d into OCA:master Aug 7, 2021
@MiquelRForgeFlow MiquelRForgeFlow deleted the master-add-rebase_bot branch August 9, 2021 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LoisRForgeFlow LoisRForgeFlow LoisRForgeFlow left review comments

@legalsylvain legalsylvain legalsylvain approved these changes

@sbidoul sbidoul Awaiting requested review from sbidoul

Assignees
No one assigned
Labels
Bot Task A task the bot does or should do enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@MiquelRForgeFlow @sbidoul @pedrobaeza @legalsylvain @bealdav @LoisRForgeFlow