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.

Sign up for GitHub

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

Jump to bottom

yessql.db -> OrchardCore.db #7446

Merged
merged 16 commits into from
Jan 19, 2024
Merged

yessql.db -> OrchardCore.db #7446

merged 16 commits into from
Jan 19, 2024

Conversation

hishamco
Copy link
Member

No description provided.

@hishamco
Copy link
Member Author

IMHO yessql doesn't make sense here, seems it's copied from YesSQL repo since the beginning and no one was care about the name ;)

deanmarcussen
deanmarcussen reviewed Oct 30, 2020
View reviewed changes
Copy link
Member

@deanmarcussen deanmarcussen left a comment

Choose a reason for hiding this comment

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

I'm not sure if you realise, but this breaks every orchard core installation using Sqlite.

@hishamco
Copy link
Member Author

but this breaks every orchard core installation using Sqlite

Please don't tell me that YesSQL is using this name?!!

@hishamco
Copy link
Member Author

AFAIK YesSQL has UseSqLite which accepts a connectionString, could please tell me how this break the OC installation, may be there's something I don't know about

@agriffard
Copy link
Member

It breaks the instances already setup because they have to rename the file to the new name.

@hishamco
Copy link
Member Author

It breaks the instances already setup because they have to rename the file to the new name.

Aha, I understand what Dean mean now

If the issue is the backward compatibility we can rename the file, or just leave the current name if all agrees

@deanmarcussen
Copy link
Member

Leave current name gets my vote.

Risky to try and rename files when there is no need

@hishamco
Copy link
Member Author

Ok

@hishamco hishamco closed this Oct 30, 2020
@sebastienros
Copy link
Member

Isn't the connection string for sqlite stored in the config too? We might want to think about it, the same way it's done for other dbs, since it will also allow to set custom settings for sqlite. And that would then allow to change the name of the file.

@hishamco hishamco reopened this Oct 31, 2020
@hishamco
Copy link
Member Author

Reopen this to think and consider what Seb is mentioned previously this will not only allow us to rename the file, but change the database itself when it's required. I think it's a good enhancement to consider

@hishamco
Copy link
Member Author

hishamco commented Dec 2, 2020

@sebastienros the SQLite database name is configurable now!!

@Skrypt
Copy link
Contributor

Skrypt commented Dec 2, 2020

Unit Test failing.

@hishamco
Copy link
Member Author

hishamco commented Dec 2, 2020

Unit Test failing.

Oops, I didn't notice that, I will check them now ...

Thanks Jasmin

@Skrypt Skrypt requested a review from deanmarcussen December 2, 2020 20:31
@Skrypt Skrypt requested a review from sebastienros December 20, 2020 18:10
@hishamco
Copy link
Member Author

I faced a lot of issues in SQLiteStudio due to the name of the tenant for all the instances, I will update this PR

@hishamco hishamco requested a review from agriffard as a code owner May 29, 2023 11:55
@hishamco
Copy link
Member Author

This should work perfectly now!! with backward compatibility is taken in mind

Piedone
Piedone requested changes Jan 11, 2024
View reviewed changes
src/docs/reference/core/Data/README.md Outdated Show resolved Hide resolved
src/docs/releases/1.9.0.md Outdated Show resolved Hide resolved
src/docs/releases/1.9.0.md Outdated Show resolved Hide resolved
@hishamco hishamco requested a review from Piedone January 12, 2024 19:03
Piedone
Piedone requested changes Jan 14, 2024
View reviewed changes
src/docs/releases/1.9.0.md Outdated Show resolved Hide resolved
@hishamco hishamco requested a review from Piedone January 14, 2024 21:11
Piedone
Piedone approved these changes Jan 14, 2024
View reviewed changes
Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

Anybody else with any comments, @sebastienros @Skrypt?

@Skrypt
Copy link
Contributor

Skrypt commented Jan 15, 2024

As long as it is documented; it should be fine.

@hishamco
Copy link
Member Author

As long as it is documented; it should be fine.

Ya it's

@hishamco hishamco merged commit 43a7a2d into main Jan 19, 2024
3 checks passed
@hishamco hishamco deleted the hishamco/db branch January 19, 2024 12:02
@microposmp microposmp mentioned this pull request Jan 20, 2024
Closed
@MikeAlhayek
Copy link
Member

@hishamco this caused a problem for me. All my local site that were using Sqllite are no longer working. I think it's because it is now looking for a database named "OrchardCore" but I was using "yessql".

Here is what you should do here. When creating new sites that use Sqllite, save "DatabaseName": "OrchardCore.db", to the shellSettings. If the shellsettings does not have DatabaseName value, then use yessql.db. This way we keep it backward compatible and only change it for new tenants.

@MikeAlhayek MikeAlhayek mentioned this pull request Jan 23, 2024
Closed
@hishamco
Copy link
Member Author

I replied on the newly created issue

sebastienros added a commit that referenced this pull request Jan 25, 2024
@sebastienros
Revert "yessql.db -> OrchardCore.db (#7446)"
146efc5 
This reverts commit 43a7a2d.

# Conflicts:
#	src/OrchardCore/OrchardCore.Data.YesSql/Options/SqliteOptions.cs
sebastienros added a commit that referenced this pull request Jan 25, 2024
@sebastienros
Revert "yessql.db -> OrchardCore.db (#7446)" (#15167)
ea5ada3 
This reverts commit 43a7a2d.

# Conflicts:
#	src/OrchardCore/OrchardCore.Data.YesSql/Options/SqliteOptions.cs
Skrypt pushed a commit that referenced this pull request Jan 25, 2024
@hishamco @Skrypt
yessql.db -> OrchardCore.db (#7446)
747068e
@github-actions github-actions bot mentioned this pull request Feb 1, 2024
Closed
hishamco added a commit that referenced this pull request Feb 1, 2024
@hishamco
yessql.db -> OrchardCore.db (#7446)
7777daf
hishamco pushed a commit that referenced this pull request Feb 1, 2024
@sebastienros @hishamco
Revert "yessql.db -> OrchardCore.db (#7446)" (#15167)
6fb5c13 
This reverts commit 43a7a2d.

# Conflicts:
#	src/OrchardCore/OrchardCore.Data.YesSql/Options/SqliteOptions.cs
urbanit pushed a commit to urbanit/OrchardCore that referenced this pull request Mar 18, 2024
@hishamco @urbanit
yessql.db -> OrchardCore.db (OrchardCMS#7446)
434a64f
urbanit pushed a commit to urbanit/OrchardCore that referenced this pull request Mar 18, 2024
@sebastienros @urbanit
Revert "yessql.db -> OrchardCore.db (OrchardCMS#7446)" (OrchardCMS#15167
3decda2 
)

This reverts commit 43a7a2d.

# Conflicts:
#	src/OrchardCore/OrchardCore.Data.YesSql/Options/SqliteOptions.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebastienros sebastienros sebastienros left review comments

@deanmarcussen deanmarcussen deanmarcussen left review comments

@Piedone Piedone Piedone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@hishamco @agriffard @deanmarcussen @sebastienros @Skrypt @Piedone @MikeAlhayek