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

service: handle string run cmd #15120

Merged

Conversation

apainintheneck
Copy link
Contributor

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This is a very bad bug that snuck in after #15007. Essentially, it didn't come up during testing because of a bug in my testing script. The Service#run handles strings by turning them into arrays. The string argument gets turned into an array internally but we skip that to preserve all args in the @run_params variable. That means that we have to handle strings when deserializing too.

This affects 58 services by my count and makes the services impossible to install correctly since the path deserialization doesn't work at all.

appium
arangodb
artifactory
asimov
auditbeat
automysqlbackup
bazarr
beanstalkd
bitcoin
bitlbee
black
blazegraph
carrot2
cassandra-reaper
chronograf
clash
clipper
code-server
collector-sidecar
couchdb-lucene
couchdb
daemontools
davmail
docuum
filebeat
freediameter
fuseki
gearman
groestlcoin
heartbeat
icecream
immudb
influxdb
ircd-hybrid
jackett
jobber
kibana@6
knot
lldpd
logstash
metabase
metricbeat
mpdas
nats-server
nats-streaming-server
openiked
opensearch-dashboards
opensearch
opentsdb
orientdb
packetbeat
pdns
pdnsd
rabbitmq
redshift
tailscale
tor
trezor-bridge
yorkie

I've updated the testing script from before to not skip services without commands which was something I did early on because of one false positive. I also added some more testing around the different run command parameter types.

@apainintheneck apainintheneck added bug Reproducible Homebrew/brew bug critical Critical change which should be shipped as soon as possible. labels Apr 1, 2023
@apainintheneck apainintheneck self-assigned this Apr 1, 2023
This was not handled at all during deserialization.
The string argument gets turned into an array internally
but we skip that to preserve all args in the @run_params
variable. That means that we have to handle strings when
deserializing too.
@apainintheneck apainintheneck added the install from api Relates to API installs label Apr 1, 2023
@MikeMcQuaid
Copy link
Member

Thanks @apainintheneck! FYI if you're marking something like this critical and it fixes a regression: I'd just self-merge.

@MikeMcQuaid
Copy link
Member

@apainintheneck does this fix #15113?

@MikeMcQuaid MikeMcQuaid merged commit 6fef708 into Homebrew:master Apr 3, 2023
24 checks passed
@apainintheneck
Copy link
Contributor Author

@apainintheneck does this fix #15113?

No, the solution there is to stop using bin in the service block. I found this because I was debugging the Cellar problem so there is that.

@github-actions github-actions bot added the outdated PR was locked due to age label May 4, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 4, 2023
@apainintheneck apainintheneck deleted the handle-service-run-cmd-string branch May 14, 2023 19:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Reproducible Homebrew/brew bug critical Critical change which should be shipped as soon as possible. install from api Relates to API installs outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants