Skip to content

[IMP] base+doc - #795

Merged
pedrobaeza merged 2 commits into
OCA:10.0from
Tecnativa:10.0-base-imp
Apr 15, 2017
Merged

[IMP] base+doc#795
pedrobaeza merged 2 commits into
OCA:10.0from
Tecnativa:10.0-base-imp

Conversation

@pedrobaeza

@pedrobaeza pedrobaeza commented Apr 15, 2017

Copy link
Copy Markdown
Member
  • base: Preserve 'birthdate' column for not losing data.
  • doc: Include 9.0 module list, not 10.0 one.
  • doc: Remove 'No change' state, as it's mixed frequently with 'Nothing to do'

@pedrobaeza

Copy link
Copy Markdown
Member Author

cc @Tecnativa

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

One question I have now: shouldn't this have use_env=False or declare env argument?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I answer myself: yes. Changing it.

* base: Preserve 'birthday' column for not losing data.
* doc: Include 9.0 module list, not 10.0 one.
* doc: Remove 'No change' state, as it's mixed frequently with 'Nothing to do'
With the change in openupgradelib, now use_env is by default True,
so we need to explicitly say no to this.
@pedrobaeza

Copy link
Copy Markdown
Member Author

Merging this one optimistically

@pedrobaeza
pedrobaeza merged commit 4bd9b4b into OCA:10.0 Apr 15, 2017
@pedrobaeza
pedrobaeza deleted the 10.0-base-imp branch April 15, 2017 17:59
@StefanRijnhart

Copy link
Copy Markdown
Member

@pedrobaeza I'd like to keep the rule that no one merges their own work, as Hintjens laid out in his explanation of optimistic merging.

@StefanRijnhart

Copy link
Copy Markdown
Member

@pedrobaeza you see, effectively the merger is the first one to notice problems with the code and propose fixes for them.

@StefanRijnhart

Copy link
Copy Markdown
Member

@pedrobaeza Up until now we always listed the modules of the new edition. I don't like how we suddenly switch to listing the modules of the previous edition. Sure, there are pros but there are cons as well.

A real improvement would be to add both the new modules and the obsolete ones, and indicate with a little icon to which category they belong. What do you think?

@pedrobaeza

Copy link
Copy Markdown
Member Author

OK, I will let you merge, but I don't agree with having the list of new modules. What you want to see in the coverage is what modules that you have in your 9.0 installation are covered by OpenUpgrade. New modules simply are not useful. You can install them when the DB is migrated, but nothing more. Sometimes, a module is renamed, but you can reflect this in the corresponding old module name row.

@pedrobaeza

Copy link
Copy Markdown
Member Author

Anyway, I had Holger's approval, so it's not a blind optimistic merge.

@StefanRijnhart

Copy link
Copy Markdown
Member

@pedrobaeza you don't like to see the new modules in the list of all the modules with a clear marker that they are new modules? Do you mean you would vote against it if I proposed it?

@pedrobaeza

Copy link
Copy Markdown
Member Author

As stated, I don't see any sense on having modules that you don't have in your 9.0 installation. What you want to see is the coverage of the migration, so you look for the modules you have installed, and see if they are "Done", "Nothing to do", or even renamed to a brand new module name in 10.0, not the contrary.

Which purpose do you have for having the new module names in this list?

@StefanRijnhart

Copy link
Copy Markdown
Member

It is a user vs. developer perspective, I believe. The new module names guide the developers to look for what has not yet been covered, like a long term to-do list. That makes sense, because the migration analysis and the migration scripts itself are also arranged in exactly that way. From that perspective, having the old module names in the table without a marker that they were removed may simply be confusing for developers who look for these modules in the openupgrade file tree where they cannot be found.

I think there is a lot to say to show the old modules in the list so that we can help estimate the impact of a migration, including a mapping of old to new modules and possible loss of functionality.
I also think there is a lot to say to show the new modules in the list, so that we can have a reverse mapping from new module name to the old one, or indicate that a module provides new functionality. And have a consistent mapping between the coverage page and the list of available addons in a release.

@pedrobaeza

Copy link
Copy Markdown
Member Author

Sorry, but I'm thinking also as a developer, and the coverage list I need is the old one, not the new one! I take my 9.0 installation and look which migration scripts I need to create because they are not created yet. On previous versions, I had to dig in apriori.py file several times to see that some modules that are with the new name doesn't need any migration (of the module itself or a migration script). Renamed modules from one version to another can be better documented putting in the coverage documentation that has been renamed, and then that you need to see for possible migration script in the new module name. It can be done this way

+-----------------------------------+-----------------------------------+
| module_a                          | Done                              |
+-----------------------------------+-----------------------------------+
| module_b (renamed to module_c)    | Nothing to do                     |
+-----------------------------------+-----------------------------------+

Removed modules from version 9.0 to version 10.0 can also be documented this way:

+-----------------------------------+-----------------------------------+
| module_removed                    | Forward-port in OCA/<project>     |
+-----------------------------------+-----------------------------------+

With a new 10.0 module, there's nothing to document, as the DB once migrated, you are free to install it if the new feature is up to your convenience.

Maybe a fresh third opinion is good here. @hbrunn @jbeficent what do you think?

@StefanRijnhart

Copy link
Copy Markdown
Member

Let's continue the discussion on #798

@pedrobaeza

Copy link
Copy Markdown
Member Author

OK, I see.

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.

3 participants