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

Fix ExternalAuthenticationMigrations namespace #17370

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hishamco
Copy link
Member

No description provided.

@MikeAlhayek
Copy link
Member

@hishamco you can't rename the migration file or it's name space as this will treat the migration as a new migration and it will run as a new migration (even if the logic ran before).

@hishamco
Copy link
Member Author

Right, that's why I need this for 3.0.0, we need to make everything consistent across the code-base

@MikeAlhayek
Copy link
Member

It does not add any value. There is no point of this change which will cause every tenant to run this migration which could potentially case issue with no value added beside a name change.

@hishamco
Copy link
Member Author

This way if there's a historical issue either in migrations or service we can't fix any anymore, that's why major releases open the possibility of breaking things

@gvkries
Copy link
Contributor

gvkries commented Jan 24, 2025

I agree with @MikeAlhayek, this is not worth the possible problems.

@hishamco
Copy link
Member Author

That means if we made a mistake accidentally in a previous release, there's no way to fix it. There are many things need to be aligned from the beginning but we hold until a major release, so we don't break anyone

@MikeAlhayek
Copy link
Member

@hishamco can we close this?

@hishamco
Copy link
Member Author

I noticed @sebastienros commenting on #17414 and #17189 which are quite similar. I know changing the namespace is critical in this case but I'm still thinking have a proper namespace for APIs is something valuable especially in any frameworks

@sebastienros if you have a better option please let me know, otherwise feel free to close

@gvkries
Copy link
Contributor

gvkries commented Feb 21, 2025

This is different from the other PRs, because it is only about an internally used namespace which is not perfect. This namespace is not used anywhere. And additionally it is moving a data migration, which causes updates to the database without a real need.

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