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

Remove 'Storage' from OC.Media.Azure feature Id #11992

Closed
wants to merge 3 commits into from

Conversation

hishamco
Copy link
Member

No description provided.

@hishamco hishamco requested a review from jtkech July 10, 2022 01:14
@jtkech
Copy link
Member

jtkech commented Jul 10, 2022

Yes you are right, I asked to do it like this for the Media.AmazonS3 module, if you look at the comments of the related merged PR.

But we didn't update Media.Azure.Storage to Media.Azure as it would be a breaking change ;)

@hishamco
Copy link
Member Author

We could mentioned this at breaking changes section in the module's README

@hishamco hishamco added the breaking change 💥 Issues or pull requests that introduces breaking change(s) label Jul 12, 2022
@hishamco hishamco requested a review from agriffard as a code owner July 12, 2022 14:37
@Skrypt
Copy link
Contributor

Skrypt commented Jul 12, 2022

Provide a migration path. A SQL Query or something for people to know what to do. Maybe just enabling back that feature but it will also add a new feature in the database while the older feature will still be there. It's not a clean migration path.

@hishamco
Copy link
Member Author

While this is a feature id, I'm not sure what's the suitable way for migration. @jtkech any suggestion?

@Piedone
Copy link
Member

Piedone commented Jan 19, 2024

I don't really see much point in changing the ID anyway, but I'm adding OrchardCore.Media.Azure.ImageSharpImageCache here: #15028. So, the distinction of the original ID will be necessary and I think this can be closed.

@hishamco
Copy link
Member Author

It's fine for OrchardCore.Media.Azure.ImageSharpImageCache while it's Azure related feature, but in terms of storage we should have Azure & Amazon

Either we accept this if all agree or do it before 2.0.0

@Piedone
Copy link
Member

Piedone commented Jan 21, 2024

But what's the reason to change the ID here, what benefit does it bring? I only see a breaking change here without a clear goal, and after #15028 is merged, a confusing feature ID.

@hishamco
Copy link
Member Author

But what's the reason to change the ID here, what benefit does it bring?

Consistency

For OrchardCore.Media.Azure.ImageSharpImageCache I prefer OrchardCore.Media.Azure.ImageCache

No rush we can defer this to 2.0.0 while it's breaking change

@Piedone
Copy link
Member

Piedone commented Jan 21, 2024

What would it be consistent with, exactly? If it were a single feature in the module then I get it but then again it won't be. And having OrchardCore.Media.Azure and OrchardCore.Media.Azure.ImageCache would imply the latter depending on or being part of the former. This will be the case once my PR is merged, but wouldn't be if OrchardCore.Media.Azure will be the storage feature in itself.

@hishamco
Copy link
Member Author

What would it be consistent with, exactly?

With OrchardCore.Media.AmazonS3

@Piedone
Copy link
Member

Piedone commented Jan 22, 2024

That module came later so maybe it needs to be consistent with the Azure one ;). Then again, what I've written above stands. These changes won't be needed in the foreseeable future.

@hishamco
Copy link
Member Author

As JT said here maybe it's our fault at the beginning

While we accepting the breaking changes in minor release, so it's fine to fix IMHO

@Piedone
Copy link
Member

Piedone commented Jan 22, 2024

My problem is that it's not making things better.

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@hishamco
Copy link
Member Author

Closing this might revisit before 2.0.0

@hishamco hishamco closed this Mar 14, 2024
@hishamco hishamco deleted the hishamco/azure-media branch March 14, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change 💥 Issues or pull requests that introduces breaking change(s) merge conflict
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants