-
-
Notifications
You must be signed in to change notification settings - Fork 784
[18.0][IMP] Update analysis files #5144
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
Conversation
| <record id="payment_method_wire_transfer" model="payment.method"> | ||
| <field name="support_refund">none</field> | ||
| <!-- <field name="active">False</field> --> | ||
| <field name="active">False</field> |
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.
this script needs to be changed to operate on a copy of the noupdate_changes.xml file
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.
That technique will require the same some human intervention. Maybe we can use this technnique:
- Have the previous noupdate_changes.xml (we can freeze a copy like we do with
update_analysis.txt). Maybe this can be manually done when requiring to modify the file. - Get the new noupdate_changes.xml.
- Get the diff between both (if any).
- Apply the diff to the modified file.
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.
it doesn't require human intervention for cases like the current one where the changes file isn't changed, only when there are actually changes, which are rare.
the diff method can fail so many ways as soon as there are non-trivial changes made that I didn't consider this worth the effort for cases where it works when writing the script.
I don't see a problem in copying noupdate_changes.xml to noupdate_changes_work.xml, and modifying + loading that. A useful extension of the bot script would be to flag modules where this has happened and the noupdate_changes.xml file is changed.
A more radical but possibly more useful approach could be to allow to pass modifications directly to load_data in view inheritance style (because barely anyone speaks xslt). In this case, this would be
openupgrade.load_data(
env, "payment_custom", "18.0.2.0/noupdate_changes.xml",
modifications="""
<xpath expr="//record[@id='payment_method_wire_transfer']/field[@name='active']" position="replace" />
""",
)
curious what @OCA/openupgrade-maintainers think
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.
OK, let's go with that procedure:
- Don't touch in this script anything.
- When having to modify
noupdate_changes.xmlfor something, we copy it tonoupdate_changes_work.xmland load this second one. - If any diff detected in these PRs, we adjust it in the work file if needed.
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.
Implemented in #5145
Adopting the approach talked in OCA#5144 (comment)
|
Should we wait for OCA/server-tools#3325 ? |
56f2379 to
42b6ae4
Compare
|
I think there is no need to force push this PR, as the bot would have done this next week anyways. |
|
So do you mean to close it, let it open, merge it and wait for the next one...? |
|
If you haven't done the force push, I would have said to let it open and wait next week for the PR being automatically updated. |
|
Clarification: I have just rebased the branch with the GitHub interface option. |
42b6ae4 to
36491cd
Compare
Analysis or noupdate changes for modules marked as done: