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

IBX-3644: [Doc] Fixed usage text suggested dest dir path #36

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

alongosz
Copy link
Member

@alongosz alongosz commented Sep 1, 2022

Question Answer
JIRA issue IBX-3644
Type improvement
Target Ibexa version v4.2
BC breaks no

This PR aligns usage (help) text of ./bin/generate-solr-config.sh script with the changes agreed in ibexa/documentation-developer#1742

Copy link
Contributor

@vidarl vidarl left a comment

Choose a reason for hiding this comment

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

In the doc PR, it is also added information about merging in the schema changes needed by Commerce. Shouldn't this script also do that part ?

@adamwojs
Copy link
Member

adamwojs commented Sep 1, 2022

Bonus point for fixing references to "eZ Platform"/"eZ Platform Cloud" 😉

@adamwojs
Copy link
Member

adamwojs commented Sep 1, 2022

>In the doc PR, it is also added information about merging in the schema changes needed by Commerce. Shouldn't this script also do that part ?

This package is part of open source edition so it shouldn't handle anything related to content/expirance and commerce editions.

@vidarl
Copy link
Contributor

vidarl commented Sep 1, 2022

This package is part of open source edition so it shouldn't handle anything related to content/expirance and commerce editions.

Well, it wouldn't hurt if it supported an --commerce command option.
The alternative is to have an almost exact copy of this script in an commerce bundle, but in regards to maintainability I don't think that would be a good suggestion ?

@alongosz
Copy link
Member Author

alongosz commented Sep 1, 2022

This package is part of open source edition so it shouldn't handle anything related to content/expirance and commerce editions.

Well, it wouldn't hurt if it supported an --commerce command option. The alternative is to have an almost exact copy of this script in an commerce bundle, but in regards to maintainability I don't think that would be a good suggestion ?

To handle it in a clean way we would need some extension point which would delegate work to the other 1st party package scripts. Given the doc is simple and easy to apply I find it to be a bit overkill. Nevertheless contributions welcomed :) The only requirement would be that it cannot be a redundant copy, but some smart mechanism adding feature on top of the existing one.

@alongosz alongosz requested review from vidarl and adamwojs September 1, 2022 14:34
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@alongosz
Copy link
Member Author

alongosz commented Sep 1, 2022

Bonus point for fixing references to "eZ Platform"/"eZ Platform Cloud" wink

@adamwojs 720759b

@vidarl
Copy link
Contributor

vidarl commented Sep 2, 2022

To handle it in a clean way we would need some extension point which would delegate work to the other 1st party package scripts. Given the doc is simple and easy to apply I find it to be a bit overkill. Nevertheless contributions welcomed :) The only requirement would be that it cannot be a redundant copy, but some smart mechanism adding feature on top of the existing one.

If I create a PR in this very repo, adding option like the following be accepted ? :

[--ibexa-commerce]                   : Make the solr schema compatible with Ibexa Commerce. This option requires Ibexa Commerce

It is a simple solution to a simple problem. Instead of adding complexity of extension points etc.

Adding a extension point where 1st party packages may add things to the schema wouldn't be hard. But if we would need the possibility for one package to change something already defined by another package would be a mess. I think we are just better off having all the logic for the solr schema in one place

@alongosz
Copy link
Member Author

alongosz commented Sep 2, 2022

To handle it in a clean way we would need some extension point which would delegate work to the other 1st party package scripts. Given the doc is simple and easy to apply I find it to be a bit overkill. Nevertheless contributions welcomed :) The only requirement would be that it cannot be a redundant copy, but some smart mechanism adding feature on top of the existing one.

If I create a PR in this very repo, adding option like the following be accepted ? :

[--ibexa-commerce]                   : Make the solr schema compatible with Ibexa Commerce. This option requires Ibexa Commerce

It is a simple solution to a simple problem. Instead of adding complexity of extension points etc.

Adding a extension point where 1st party packages may add things to the schema wouldn't be hard. But if we would need the possibility for one package to change something already defined by another package would be a mess. I think we are just better off having all the logic for the solr schema in one place

The problem with that is that you're exposing details of a closed-source package to an open source one, which is:

  • LICENSE-wise questionable
  • not clean
  • not SOLID, because it violates Open-Closed principle.

@vidarl
Copy link
Contributor

vidarl commented Sep 2, 2022

Okay, so that is a no go then

@alongosz alongosz merged commit 8a15fe4 into main Sep 2, 2022
@alongosz alongosz deleted the ibx-3644-fix-gen-solr-config-usage-text branch September 2, 2022 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants