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

[MIG] web_pwa_oca: Migration to V13 #1624

Merged
merged 5 commits into from Jun 25, 2020
Merged

[MIG] web_pwa_oca: Migration to V13 #1624

merged 5 commits into from Jun 25, 2020

Conversation

AntoniRomera
Copy link

No description provided.

@OCA-git-bot
Copy link
Contributor

Hi @eLBati,
some modules you are maintaining are being modified, check this out!

@eLBati
Copy link
Member

eLBati commented Jun 23, 2020

@AntoniRomera thanks.
It seems some 12.0 commits are missing: https://github.com/OCA/web/commits/12.0/web_pwa_oca
Did you follow https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-13.0#technical-method-to-migrate-a-module-from-120-to-130-branch ?

@AntoniRomera
Copy link
Author

AntoniRomera commented Jun 23, 2020 •
edited

@AntoniRomera thanks.
It seems some 12.0 commits are missing: https://github.com/OCA/web/commits/12.0/web_pwa_oca
Did you follow https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-13.0#technical-method-to-migrate-a-module-from-120-to-130-branch ?

@eLBati yes, I followed step by step.
But I can see the 3 commits in this timeline or I'm wrong.

@eLBati
Copy link
Member

eLBati commented Jun 24, 2020

@AntoniRomera my bad, I saw it wrong.

Shouldn't the last 2 pre-commit fixes be included in black, isort, prettier?

[IMP] web_pwa_oca: black, isort, prettier
c2c283c 
pre-commit fixes

pre-commit fixes 2
@AntoniRomera
Copy link
Author

@AntoniRomera my bad, I saw it wrong.

Shouldn't the last 2 pre-commit fixes be included in black, isort, prettier?

@eLBati done!

eLBati
eLBati approved these changes Jun 24, 2020
View reviewed changes
Copy link
Member

@eLBati eLBati left a comment

Choose a reason for hiding this comment

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

code review

@eLBati
Copy link
Member

eLBati commented Jun 24, 2020

@AntoniRomera did you also test this on mobile?

@AntoniRomera
Copy link
Author

@AntoniRomera did you also test this on mobile?

Yes I test in my mobile (iPhone 7 plus) and work correctly

eLBati
eLBati suggested changes Jun 24, 2020
View reviewed changes
web_pwa_oca/README.rst Show resolved Hide resolved
eLBati
eLBati approved these changes Jun 24, 2020
View reviewed changes
Copy link
Member

@eLBati eLBati left a comment

Choose a reason for hiding this comment

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

Thanks!

I think Delete incorrect documentation can be included in Migration to 13.0

[MIG] : Migration to 13.0
8f84a42 
[DEL] web_pwa_oca: Delete incorrect documentation
mileo
mileo reviewed Jun 25, 2020
View reviewed changes
web_pwa_oca/README.rst


>>> from openupgradelib import openupgrade
>>> openupgrade.update_module_names(env.cr, [('web_pwa', 'web_pwa_oca')], merge_modules=False)
Copy link
Member

@mileo mileo Jun 25, 2020 •
edited

Choose a reason for hiding this comment

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

This is no longer necessary.

Copy link
Member

Choose a reason for hiding this comment

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

It is removed from readme/INSTALL.rst and README.rst will be re-generated by ocabot

MRomeera
MRomeera reviewed Jun 25, 2020
View reviewed changes
Copy link

@MRomeera MRomeera left a comment

Choose a reason for hiding this comment

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

Approve

mileo
mileo approved these changes Jun 25, 2020
View reviewed changes
Copy link
Member

@mileo mileo left a comment

Choose a reason for hiding this comment

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

LGTM, tested in production.

@eLBati
Copy link
Member

eLBati commented Jun 25, 2020

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Sorry @eLBati you are not allowed to merge.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@eLBati
Copy link
Member

eLBati commented Jun 25, 2020

We need a @OCA/web-maintainers here, or an improvement for @OCA-git-bot to allow module's maintainers to merge migration PRs (@sbidoul , was this already discussed?)

@sbidoul
Copy link
Member

sbidoul commented Jun 25, 2020

Yes, an improvement to the bot to allow maintainers to migrate their own modules has been discussed.
I just created OCA/oca-github-bot#122 to track this.

@sbidoul
Copy link
Member

sbidoul commented Jun 25, 2020

I'm not a web maintainer but the migration looks simple enough and all review comments seem to have been addressed, so
/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 13.0-ocabot-merge-pr-1624-by-sbidoul-bump-patch, awaiting test results.

@eLBati
Copy link
Member

eLBati commented Jun 25, 2020

@sbidoul thanks

@OCA-git-bot OCA-git-bot merged commit 68b95da into OCA:13.0 Jun 25, 2020
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at c3b3c8b. Thanks a lot for contributing to OCA. ❤️

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

@eLBati eLBati eLBati approved these changes

@pedrobaeza pedrobaeza pedrobaeza left review comments

@mileo mileo mileo approved these changes

@marcelsavegnago marcelsavegnago marcelsavegnago approved these changes

@FernandoRomera FernandoRomera FernandoRomera approved these changes

@MRomeera MRomeera MRomeera approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@AntoniRomera @OCA-git-bot @eLBati @sbidoul @mileo @marcelsavegnago @pedrobaeza @FernandoRomera @MRomeera @oca-travis