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

[17.0][MIG] auth_oauth_multi_token: Migration to 17.0 #742

Open
wants to merge 20 commits into
base: 17.0
Choose a base branch
from

Conversation

kobros-tech
Copy link

fixing git commits log history with keeping the changes the same for PR #694 this will enable me to migrate to 18.0

Florent de Labarre and others added 18 commits September 24, 2024 06:54
Allow multiple oauth login at the same time.
* cleanup, improve, docstrings
* add tests
Nothing special. Just making linters happy and splitting the readme.

@Tecnativa TT25619
Otherwise lookup is slow when there are many users.
Otherwise you can't delete a user.
This is cosmetic only, because this field is not used
when auth_oauth_multi_token is installed.
Currently translated at 100.0% (15 of 15 strings)

Translation: server-auth-16.0/server-auth-16.0-auth_oauth_multi_token
Translate-URL: https://translation.odoo-community.org/projects/server-auth-16-0/server-auth-16-0-auth_oauth_multi_token/it/
@timnis
Copy link

timnis commented Jan 17, 2025

Hi @CRogos could this PR merged?

Copy link
Contributor

@CRogos CRogos left a comment

Choose a reason for hiding this comment

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

Could you have a look on your commits? Some of them should be squashed, and the last commit should be splitted into:
[IMP] auth_oauth_multi_token: pre-commit auto fixes
[MIG] auth_oauth_multi_token: Migration to 17.0
https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-17.0

image

Code LGTM, but havn't tested yet.

Do you use odoo.sh? The last time I've tested this module it was not compatible with the login-as function of odoo.sh

@timnis
Copy link

timnis commented Jan 22, 2025

Hi @kobros-tech Could you have a look on your commits?

@kobros-tech kobros-tech force-pushed the 17.0-mig-auth_oauth_multi_token branch from 3aed0b7 to 6182408 Compare January 22, 2025 12:11
@kobros-tech
Copy link
Author

kobros-tech commented Jan 22, 2025

@timnis
@CRogos

Hi all, I improved the PR more, and all contributions logs are mentioned.

To have all log preserved here, there was a conflict arised and I guess the first PR was not made correctly as I had to resolve a conflict in another module which is out of scope.

If I git rid of there commits and start on top of last merged commit in version 16.0 or to keep it as it is

@kobros-tech kobros-tech force-pushed the 17.0-mig-auth_oauth_multi_token branch from eda2f56 to 497efa2 Compare January 22, 2025 12:51
@kobros-tech
Copy link
Author

@CRogos
@timnis

so far so good :)

@CRogos
Copy link
Contributor

CRogos commented Jan 22, 2025

@kobros-tech can you squash these commits as well:
image

also some of your content in the migration commit should be in the "[IMP] auth_oauth_multi_token: pre-commit auto fixes" commit as it was done in the original PR (but without the deb file)

image

@kobros-tech
Copy link
Author

@CRogos

please, make sure that the commit you mentioned that has pandoc file is not there, only the [MIG] commit which is clean as I guess.

I don't understand why should I squash commits done for 16.0 and not for 17.0, could you explain?

@CRogos
Copy link
Contributor

CRogos commented Jan 24, 2025

@kobros-tech
Squashing commits is part of the migration guide: https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-18.0
and there is an extra document, which explains what to squash and how to do it: https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate

In #694 the Commit history is according to the migration guide.
image
But this PR has the issue with the *.deb file, which is a big issue. We need to make sure that this file is not commited in any of the commits. (adding in one commit and removing it in another commit still blows up the repository size)

Nevertheless, the "[IMP] auth_oauth_multi_token: pre-commit auto fixes" commit of #694 is correct beside of the *.deb file and should also exist in this PR.

@kobros-tech
Copy link
Author

@CRogos

I need you to look at the log of commits, because the commit you are talking about is squashed already and pandoc file is not mentioned or tracked in the migration commit and you can review, thanks!

Screenshot from 2025-01-24 19-30-35
Screenshot from 2025-01-24 19-30-54

@kobros-tech
Copy link
Author

@CRogos
If you still are suspecting, I strongly ask you for the commit id that contains pandoc file here and not in other PR as I worked on it and I am quite sure that pandoc file is not tracked at all in this PR branch

@CRogos
Copy link
Contributor

CRogos commented Jan 24, 2025

@CRogos If you still are suspecting, I strongly ask you for the commit id that contains pandoc file here and not in other PR as I worked on it and I am quite sure that pandoc file is not tracked at all in this PR branch

There is no commit with pandoc file, but there is also no commit "[IMP] auth_oauth_multi_token: pre-commit auto fixes" which should be there.

image

@kobros-tech
Copy link
Author

@CRogos If you still are suspecting, I strongly ask you for the commit id that contains pandoc file here and not in other PR as I worked on it and I am quite sure that pandoc file is not tracked at all in this PR branch

There is no commit with pandoc file, but there is also no commit "[IMP] auth_oauth_multi_token: pre-commit auto fixes" which should be there.

image

thanks, now I understand what you mean.

In fact I am not used to make two commits for migration, I have a point of view that I add it in a single commit, I don't mind you if you see it necessary here.

@kobros-tech
Copy link
Author

@CRogos
let me know if I have to divide the commit into two instad of one, so that we get the module merged and get satisfied.

@CRogos
Copy link
Contributor

CRogos commented Jan 24, 2025

Yes, that is what I'm trying to explain. You need to split the commit and adapt the commit message.

@kobros-tech kobros-tech force-pushed the 17.0-mig-auth_oauth_multi_token branch from 497efa2 to 873af5f Compare January 24, 2025 18:22
@kobros-tech
Copy link
Author

@CRogos
two commits are there, clean and out of pandoc tracking

@CRogos
Copy link
Contributor

CRogos commented Jan 24, 2025

Better, but could you squash the yellow marked and correct the commit message of the last commit?
image

nolabel="1"
options="{'no_create': True, 'no_open': True}"
>
<tree limit="10">
Copy link
Contributor

Choose a reason for hiding this comment

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

This node should be changed to <list

Copy link
Author

Choose a reason for hiding this comment

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

Is list tag introduced in version 18.0 or version 17.0?

Copy link
Contributor

Choose a reason for hiding this comment

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

you are totally right... got confused, sorry.

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.

9 participants