Skip to content

Conversation

@pedrobaeza
Copy link
Member

Same as #20 with a renaming complement.

if not version:
return
# In order to migrate connector from v. 9.0 to v. 10.0, we need to set
# connector module state to 'to upgrade'
Copy link
Member

Choose a reason for hiding this comment

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

I suggest adding, 'If this is not done, all connector.* xmlids are removed due to the module renaming done by OpenUpgrade. In the future the approach sketched in #23 (comment) may provide a more generic solution.'

@coveralls
Copy link

coveralls commented Aug 29, 2017

Coverage Status

Coverage remained the same at 77.94% when pulling ad09595 on Tecnativa:10.0-queue_ou into 6c9d7b7 on OCA:10.0.

@sbidoul
Copy link
Member

sbidoul commented Aug 29, 2017

Thanks Pedro. Can you remove the trailing whitespace to make pylint happy?

SET state='to upgrade'
WHERE name='connector'
""")
from openupgradelib import openupgrade
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this going to fail if you don't have openupgradelib in your python env?

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, but it's a requirement for OpenUpgrade

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say we should not assume that ppl are going to use it to do the migration. Can't we wrap it in a try/except ImportError and log the error?

Copy link
Member

Choose a reason for hiding this comment

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

finally somebody who agrees with me

Copy link
Member Author

Choose a reason for hiding this comment

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

My explanation continues the same that in that moment. The script is not going to be executed if you are not migrating.

Copy link
Member Author

Choose a reason for hiding this comment

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

@hbrunn the problem is then you are requiring that library for the normal fresh installation of the module where most people are not going to migrate in life (bad for them).

Copy link
Member

Choose a reason for hiding this comment

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

true. But I envision a world where basically any OCA module depends on openupgradelib, then we can harness its full power when radically changing the database layout somewhere in the lifecycle of a release. Actually, I sometimes find myself avoiding big changes just because I'd hate the idea to write a script like that without openupgradelib.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, me too, but I don't want to modify the external dependencies of this module as it has been a total pain and they will put problems for that. I won't contribute anymore to this repository.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW I personally think that

  1. using openupgradelib in migration scripts is tremendously helpful
  2. it's a lightweight dependency, I see no issue to depend on it explicitly and we must put it in external dependencies when we use it

Copy link
Member

Choose a reason for hiding this comment

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

I won't contribute anymore to this repository.

@pedrobaeza I really don't understand why you react like that. This repo is one of the best managed OCA repos. And the debate was nothing else than people (not exactly newbies btw) trying to get a deep understanding of a complex issue. And a good idea emerged of the discussion.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.94% when pulling 6c2d675 on Tecnativa:10.0-queue_ou into 6c9d7b7 on OCA:10.0.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.94% when pulling 6c2d675 on Tecnativa:10.0-queue_ou into 6c9d7b7 on OCA:10.0.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.94% when pulling f642f25 on Tecnativa:10.0-queue_ou into 6c9d7b7 on OCA:10.0.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.94% when pulling f642f25 on Tecnativa:10.0-queue_ou into 6c9d7b7 on OCA:10.0.

@hbrunn hbrunn merged commit 07bcbf4 into OCA:10.0 Aug 30, 2017
@pedrobaeza pedrobaeza deleted the 10.0-queue_ou branch August 30, 2017 07:54
@hbrunn
Copy link
Member

hbrunn commented Aug 30, 2017

thanks btw, if that's not clear from the discussion above 😉

@guewen
Copy link
Member

guewen commented Aug 30, 2017

Thanks all. I'm happy we reached a common agreement and that we learned more about openupgrade in the process.

@guewen
Copy link
Member

guewen commented Aug 30, 2017

Yeah, me too, but I don't want to modify the external dependencies of this module as it has been a total pain and they will put problems for that. I won't contribute anymore to this repository.

Sorry you see it like that.

eantones pushed a commit to nuobit/queue that referenced this pull request Oct 16, 2020
romi477 pushed a commit to romi477/queue that referenced this pull request Oct 25, 2021
bizzappdev pushed a commit to BizzAppDev-Systems/queue that referenced this pull request Nov 23, 2023
bizzappdev pushed a commit to BizzAppDev-Systems/queue that referenced this pull request Nov 23, 2023
nguyenminhchien pushed a commit to nguyenminhchien/queue that referenced this pull request Nov 25, 2023
QuocDuong1306 pushed a commit to QuocDuong1306/queue that referenced this pull request Sep 19, 2024
QuocDuong1306 pushed a commit to QuocDuong1306/queue that referenced this pull request Sep 24, 2024
thienvh332 pushed a commit to thienvh332/queue that referenced this pull request Oct 7, 2024
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.

7 participants