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

fix: add jq raw output flag (#3085) #3087

Closed
wants to merge 2 commits into from
Closed

fix: add jq raw output flag (#3085) #3087

wants to merge 2 commits into from

Conversation

CorySanin
Copy link

@CorySanin CorySanin commented Apr 1, 2024 •
edited

Description
Use jq's "raw output" flag so it doesn't introduce unwanted double quotes in its output.

Closes #3085, Closes #3092, Closes #3139

  • DO make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
  • DO make sure you are making a pull request against the master branch (left side). Also you should start your branch off our master.
  • DO make sure that File Browser can be successfully built. See builds and development.
  • AVOID breaking the continuous integration build.

@CorySanin CorySanin requested a review from o1egl as a code owner April 1, 2024 22:07
Guiorgy
Guiorgy reviewed Apr 2, 2024
View reviewed changes
healthcheck.sh Outdated Show resolved Hide resolved
@Guiorgy Guiorgy mentioned this pull request Apr 2, 2024
Closed
@kloon15
Copy link
Contributor

kloon15 commented Apr 5, 2024

Genuine question, why did we stop using the hardcoded localhost/health url? Do ppl actually run this in host mode?
I build my own using custom Dockerfile and just baffled about this.

@Guiorgy
Copy link

Guiorgy commented Apr 5, 2024

@kloon15 Just look at the PR that did the change in the first place to see the reasoning: #2938

@kloon15
Copy link
Contributor

kloon15 commented Apr 5, 2024

@kloon15 Just look at the PR that did the change in the first place to see the reasoning: #2938

I dont think u should be changing IP address inside the container, just create a custom network for ur needs in docker.

@Guiorgy
Copy link

Guiorgy commented Apr 5, 2024 •
edited

@kloon15 I mean, why are you telling that to me? 😅
I just tried to answer your question. I didn't say it's a good reason...

Edit: Also, there will always be somebody that does something that "should not be done" for a reason that makes sense to them.

@CorySanin CorySanin mentioned this pull request Apr 6, 2024
Closed
@drewstopherlee drewstopherlee mentioned this pull request Apr 7, 2024
Closed
qdm12
qdm12 approved these changes Apr 12, 2024
View reviewed changes
Copy link
Contributor

@qdm12 qdm12 left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for the fix and sorry to have caused the issue!

Also my bad for not documenting more the reason of that change, more than just the error it gave me.

EDIT 2024.04.17: the reason was I needed FB to listen on 10.0.0.38 (so not localhost) so the healthcheck querying it on localhost wouldn't work. If the variable FB_ADDRESS is exposed, the healthcheck should adapt to it, so it needed a fix. Well, yes the fix missed on that jq corner case, my bad on that one.
This listening was needed I think for my host firewall to work correctly to restrict intra containers communication, or for reverse proxy reasons, not 100% sure why anymore but i was definitely needed.

@ferdinandmehlan ferdinandmehlan mentioned this pull request Apr 15, 2024
Open
@saltydk
Copy link

saltydk commented Apr 17, 2024

@qdm12 Couldn't you just override the healthcheck in the deployment of your container since it is a runtime option?

@qdm12
Copy link
Contributor

qdm12 commented Apr 17, 2024

It would be nice to merge this PR rather soon, it's only 2 lines 😉 Tagging as a reminder @o1egl thanks 👍

@saltydk I did override the healthcheck script to test it out, and thought I would contribute with a PR, I didn't foresee the impact with jq not handling empty fields properly. When you say

Couldn't you just override the healthcheck in the deployment of your container since it is a runtime option?

I could propose the exact same to you today, just override the healthcheck, but it's not really a solution, is it?

@saltydk
Copy link

saltydk commented Apr 18, 2024

It would be nice to merge this PR rather soon, it's only 2 lines 😉 Tagging as a reminder @o1egl thanks 👍

@saltydk I did override the healthcheck script to test it out, and thought I would contribute with a PR, I didn't foresee the impact with jq not handling empty fields properly. When you say

Couldn't you just override the healthcheck in the deployment of your container since it is a runtime option?

I could propose the exact same to you today, just override the healthcheck, but it's not really a solution, is it?

A user runtime based override on something fairly niche makes more sense to me than changing the image but they merged it so it is what it is. Was just wondering is all.

@Guiorgy
Copy link

Guiorgy commented Apr 18, 2024 •
edited

@saltydk If the change impacts normal operation, then yes, but this was supposed to be a change that would benefit some without downsides to the rest. Being more accessible and easy to use for more people is always good. We are all humans, we make mistakes, no need to make a mountain out of a molehill.

@o1egl
Copy link
Member

o1egl commented Apr 24, 2024

Merged as part of the #3130

@o1egl o1egl closed this Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Guiorgy Guiorgy Guiorgy left review comments

@qdm12 qdm12 qdm12 approved these changes

@o1egl o1egl Awaiting requested review from o1egl o1egl is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@CorySanin @kloon15 @Guiorgy @saltydk @qdm12 @o1egl