Skip to content

[14.0][MIG] connector#385

Merged
OCA-git-bot merged 723 commits into
OCA:14.0from
nuobit:14.0-mig-connector
Mar 26, 2021
Merged

[14.0][MIG] connector#385
OCA-git-bot merged 723 commits into
OCA:14.0from
nuobit:14.0-mig-connector

Conversation

@eantones

@eantones eantones commented Oct 16, 2020

Copy link
Copy Markdown
Contributor

@eantones

Copy link
Copy Markdown
Contributor Author

All tests passed perfectly on my test environment
I think there's something wrong with the test-dependencies.txt and travis is not able to clone the queue_job PR.
Any ideas?
Thanks! :-)

@eantones eantones mentioned this pull request Oct 19, 2020
6 tasks
Comment thread test-requirements.txt Outdated
@eantones
eantones force-pushed the 14.0-mig-connector branch 3 times, most recently from f627d40 to 82d49e7 Compare October 21, 2020 08:05
@sebastienbeau

Copy link
Copy Markdown
Member

@eantones Dependency have been merged
Can you update you commit to remove the test-dependency file ?
Thanks

@eantones

eantones commented Dec 2, 2020

Copy link
Copy Markdown
Contributor Author

@sebastienbeau done!
Thanks!

@eantones
eantones force-pushed the 14.0-mig-connector branch 2 times, most recently from d4f31f7 to 8a81c49 Compare December 2, 2020 18:37
Comment thread connector/models/checkpoint.py Outdated
from odoo import _, api, fields, models


class ConnectorCheckpoint(models.Model):

@guewen guewen Dec 3, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe the good time to remove this? #351

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

good idea !

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can this be removed as suggested?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think so, nobody is using it AFAIK.
@eantones likely this is the last bit 😉

@esakarya

Copy link
Copy Markdown

any update ?

@kittiu

kittiu commented Mar 14, 2021

Copy link
Copy Markdown
Member

@eantones please check travis and rebase. All dependencies are merged.

@rvalyi rvalyi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I checked that the dependencies didn't change much between v13 and v14. Also inside the module components and models have no reason to change to accomodate to any change in Odoo. So given tests are passing, that's LGTM.

@rousseldenis rousseldenis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code review. Maybe increasing a little bit coverage should be great.

@OCA/connector-maintainers We certainly should switch to main branches including non migrated modules from past versions in order to avoid such PR with 721 commits.

@guewen

guewen commented Mar 26, 2021

Copy link
Copy Markdown
Member

@OCA/connector-maintainers We certainly should switch to main branches including non migrated modules from past versions in order to avoid such PR with 721 commits.

+1
I did this for the previous versions, but I missed the occasion to ensure this for 14.0

@guewen

guewen commented Mar 26, 2021

Copy link
Copy Markdown
Member

@eantones I'm going to propose a PR on your PR to remove checkpoints

Alternatives such as activities can be used.
@guewen

guewen commented Mar 26, 2021

Copy link
Copy Markdown
Member

@eantones can you include nuobit#1 please? Then we are good to merge :)

@eantones

Copy link
Copy Markdown
Contributor Author

@guewen done! :-)

@eantones

Copy link
Copy Markdown
Contributor Author

@guewen @rousseldenis regarding the 721 commits, you're right, there's too many, I can make a squash

@rousseldenis

Copy link
Copy Markdown
Contributor

I can make a squash

No! You'll loose the commit history and that is not wanted.

@rousseldenis

Copy link
Copy Markdown
Contributor

@guewen @rousseldenis regarding the 721 commits, you're right, there's too many, I can make a squash

There are two ways of doing main branches :

  • Creating the branch with no module at all from scratch (actual OCA convention apart some repos). All migration PR are created with the whole commit history like here.
  • Creating the new main branch (14.0) based on previous one (13.0). So, the commit history is preserved between them. The advantage is that migration PR would contain only the new commits and is more readable.

@eantones

Copy link
Copy Markdown
Contributor Author

@rousseldenis yes I know, so how do you suggest to reduce the commits?

@rousseldenis

Copy link
Copy Markdown
Contributor

@rousseldenis yes I know, so how do you suggest to reduce the commits?

Nope, this will be merged as is, it's ok

@eantones

Copy link
Copy Markdown
Contributor Author

@rousseldenis ok, so you meant that we cannot do anything now because this should have been done on 14.0 branch creation right?

@guewen guewen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks ❤️
Waiting for the build before merging.

@guewen

guewen commented Mar 26, 2021

Copy link
Copy Markdown
Member

ok, so you meant that we cannot do anything now because this should have been done on 14.0 branch creation right?

yep

@eantones

Copy link
Copy Markdown
Contributor Author

Thanks :-)

@simahawk

Copy link
Copy Markdown
Contributor

@guewen @rousseldenis regarding the 721 commits, you're right, there's too many, I can make a squash

There are two ways of doing main branches :

* Creating the branch with no module at all from scratch (actual OCA convention apart some repos). All migration PR are created with the whole commit history like here.

* Creating the new main branch (14.0) based on previous one (13.0). So, the commit history is preserved between them. The advantage is that migration PR would contain only the new commits and is more readable.

Workaround: before submitting a migration PR open another one where the module is ported as is from another version and made uninstallable.

@guewen

guewen commented Mar 26, 2021

Copy link
Copy Markdown
Member

/ocabot merge nobump

@OCA-git-bot

Copy link
Copy Markdown
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-385-by-guewen-bump-nobump, awaiting test results.

@OCA-git-bot
OCA-git-bot merged commit 23477ec into OCA:14.0 Mar 26, 2021
@OCA-git-bot

Copy link
Copy Markdown
Contributor

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

@guewen

guewen commented Apr 23, 2021

Copy link
Copy Markdown
Member

Hi, I realize that test_component and test_connector add-ons are not included. You may have missed them, but they are what ensure we have no regression

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.