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

Skipped generating solrconfig.xml file copies in generate-solr-config.sh #58

Merged
merged 2 commits into from
Nov 11, 2023

Conversation

adamwojs
Copy link
Member

@adamwojs adamwojs commented Nov 4, 2023

Question Answer
JIRA issue N/A
Type bug
Target Ibexa version v4.5
BC breaks no

Skipped generating solrconfig.xml file copies in bin/generate-solr-config.sh: solrconfig.xml.bak and solrconfig.xml.bak2

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (main for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping for example @ibexa/php-dev for back-end changes and/or @ibexa/javascript-dev for front-end changes).

@mnocon
Copy link
Contributor

mnocon commented Nov 8, 2023

I'm not sure about this one, I think it was made this way to make it work on OSX as well (https://stackoverflow.com/questions/4247068/sed-command-with-i-option-failing-on-mac-but-works-on-linux)

@adamwojs
Copy link
Member Author

adamwojs commented Nov 8, 2023

Then we should clean up backup files after sed command 😉

@alongosz
Copy link
Member

alongosz commented Nov 8, 2023

I'm not sure about this one, I think it was made this way to make it work on OSX as well (https://stackoverflow.com/questions/4247068/sed-command-with-i-option-failing-on-mac-but-works-on-linux)

Interesting... Maybe then sed -i''?
Or use good version of sed:

brew install gnu-sed
alias sed=gsed

@alongosz
Copy link
Member

alongosz commented Nov 8, 2023

But on the other hand this should be used by CI only. Can we actually move to docker when trying to run this on local machines? That CI automation ran locally has its disadvantages.

@mnocon
Copy link
Contributor

mnocon commented Nov 8, 2023

But on the other hand this should be used by CI only. Can we actually move to docker when trying to run this on local machines? That CI automation ran locally has its disadvantages.

It's mentioned directly in the Platform.sh configuration files:
https://github.com/ibexa/post-install/blob/main/resources/platformsh/ibexa-commerce/4.6.x-dev/.platform/services.yaml#L54-L58

and can be run by partners - I don't think they will see your comment about gnu-sed 😛

@alongosz
Copy link
Member

alongosz commented Nov 8, 2023

But on the other hand this should be used by CI only. Can we actually move to docker when trying to run this on local machines? That CI automation ran locally has its disadvantages.

It's mentioned directly in the Platform.sh configuration files: https://github.com/ibexa/post-install/blob/main/resources/platformsh/ibexa-commerce/4.6.x-dev/.platform/services.yaml#L54-L58

and can be run by partners - I don't think they will see your comment about gnu-sed 😛

Meh :)
I thought it was for CI only. Nvm then.

@webhdx
Copy link
Contributor

webhdx commented Nov 8, 2023

Maybe we should just recommend installing and aliasing sed? This could be a requirement. sed is a commonly used tool.

Copy link

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

@adamwojs
Copy link
Member Author

Now backup copies are automatically removed. Merging.

@adamwojs adamwojs merged commit 98dbd6b into 4.5 Nov 11, 2023
19 checks passed
@adamwojs adamwojs deleted the adamwojs-patch-1 branch November 11, 2023 09:42
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.

5 participants