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

Capture instance name #116

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Capture instance name #116

wants to merge 2 commits into from

Conversation

zackmay
Copy link

@zackmay zackmay commented Jul 17, 2024

added a safety net if folks forget to add varnish instance name.

@zackmay zackmay requested a review from gquintard July 17, 2024 14:01
@zackmay zackmay self-assigned this Jul 17, 2024
@zackmay zackmay linked an issue Jul 17, 2024 that may be closed by this pull request
@gquintard
Copy link
Contributor

@hermunn , I could use your opinion on this.
On the one hand, I like how straightforward the code is, it makes a low-effort, high probability attempt at fixing the initial command.
On the other hand, I feel the amount of magic and lack of transparency disturbing.

My knee-jerk reaction would be to run varnishadm ping (possibly with the user-provided -n argument) and warn if it fails ("is varnish running, do you have the right permissions, etc."), but that's actually less useful than the current proposal

@hermunn
Copy link
Contributor

hermunn commented Jul 18, 2024

I think @gquintard has a good point. I think maybe a combination of the ideas would be the following:

  • If no -n argument is given, you check if varnishadm ping (with the -n argument) gives a good answer. If it does not, you use this PR's idea to suggest what the users should do: You need to / should consider to specify the instance using -n, and the following values seem to be reasonable: and then a list of the observed varnishd instances which have a -n argument. If there are no varnishd running, you finish this script, for the sake of other information gathering.
  • If there is a -n argument, but varnishadm ping -n does not work, report that and suggest other -n values that might be better, but run the rest of the script.

In short, I think that the idea of figuring out the right -n is very good, but maybe it can make even better than what is in the gather. I would also be fine with merging this or something very similar to what we have, and expand on it later.

@gquintard
Copy link
Contributor

@zackmay, bump

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.

safety net for instance name
3 participants