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.

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

[15.0][MIG] auth_totp: migration script #3480

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

MiquelRForgeFlow
Copy link
Contributor

Replaces #3266.

@MiquelRForgeFlow MiquelRForgeFlow added this to the 15.0 milestone Jul 27, 2022
@MiquelRForgeFlow MiquelRForgeFlow merged commit 5136c98 into OCA:15.0 Jul 27, 2022
@MiquelRForgeFlow MiquelRForgeFlow deleted the v15_mig_auth_totp branch July 27, 2022 15:01
@legalsylvain
Copy link
Contributor

legalsylvain commented Jul 27, 2022

Hi.

  • what it the reason to add an explicit space before "done" ? (CI seems working with / without the space).
  • could you call /ocabot migration when your create your fork, to avoid to have obsolete information in Migration to version 15.0 #3287
  • if the space is important, could be great to say the information to the contributor of OpenUpgrade. (to avoid this extra work for you).

thanks.

@OCA OCA deleted a comment from OCA-git-bot Jul 27, 2022
@MiquelRForgeFlow
Copy link
Contributor Author

I have already decided to avoid doing the superseding if it's just to add the space. Thus, in this PR, I improved some things, like adding the create_date in the table.

@legalsylvain
Copy link
Contributor

I have already decided to avoid doing the superseding if it's just to add the space. Thus, in this PR, I improved some things, like adding the create_date in the table.

  1. Please respect commit. It's not possible to know what you changed from the original commit from @sang250399 In that case : 1 commit from @sang250399 + one commit from @MiquelRForgeFlow
  2. I don't understand why you don't review original PR. (asking for changes if required).

thanks.

@MiquelRForgeFlow
Copy link
Contributor Author

It's not possible to know what you changed

Well, you can compare both with attention and you may find the changes.

  1. Because one thing is adding more things, which justifies another commit, and other thing, it's just minor fixes in a query or something like that, which in my opinion, doesn't justify another commit.
  2. Because asking changes requires the contributor do the change, which will require time. If I don't want to wait, I prefer to proceed this way. And thus, I can proceed to review and merge other PRs, because the modules are linked by a dependency hierarchy. If I don't proceed with this fast way, then all the process of review and merge gets stuck or paused in a long process.

@MiquelRForgeFlow
Copy link
Contributor Author

/ocabot migration auth_totp

@OCA-git-bot
Copy link
Contributor

The migration issue (#3287) has been updated to reference the current pull request.
however, a previous pull request was referenced : #3266.
Perhaps you should check that there is no duplicate work.
CC : @sang250399

@legalsylvain
Copy link
Contributor

Well, you can compare both with attention and you may find the changes.

Indeed, but it is extra work to review to make a meld. if you add an extra commit, the review is trivial.

Because one thing is adding more things, which justifies another commit, and other thing, it's just minor fixes in a query or something like that, which in my opinion, doesn't justify another commit.

well, that's not right. When I see

image

i assume that @sang250399 makes the changes, and you only cherry picked the commit.
I don't see any problem to add an extra commit "[FIX] add create_date in the migration process..."

Because asking changes requires the contributor do the change, which will require time.

Please try at least ! If there is no answer, from the contributor, you can supersed.

Note:
I am not saying this to denigrate your work. You provide an awesome contribution on Openupgrade. I'm just trying to think of how to integrate newcomers into this project.

If I put myself in their shoes: if I make a lot of pull requests, and I don't get any review, and suddenly my pull request is closed, and my commit modified and merged into another pull request:

  • I might get discouraged and don't contribute
  • I won't make any progress because seeing the changes made by the second pull request will not be obvious. (and it is not possible to talk about the changes...)

In the other hand, I understand that you could be blocked by such dependencies. Maybe we could talk on that topic in the next Openupgrade meetings.

kind regards.

@pedrobaeza
Copy link
Member

Well, let's ask if @sang250399 feels that way. I won't get angry as Miquel is preserving the authorship and just getting things done.

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.

5 participants