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

Remove cachemgr.cgi tool #1542

Closed
wants to merge 8 commits into from
Closed

Remove cachemgr.cgi tool #1542

wants to merge 8 commits into from

Conversation

yadij
Copy link
Contributor

@yadij yadij commented Oct 25, 2023

No description provided.

@yadij yadij added the feature maintainer needs documentation updates for merge label Oct 25, 2023
kinkie
kinkie previously approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@kinkie kinkie left a comment

Choose a reason for hiding this comment

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

big cleanup!

rousskov
rousskov previously approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Thank you very much for removing this tool!

If this change should be reflected in doc/release-notes/release-7.sgml.in, please adjust accordingly. FWIW, recent commit 80cba6d did adjust those release notes when removing squidclient.

If possible, please adjust the following three leftovers as well:

INSTALL:27:the tools/cachemgr.cgi program into your httpd server's cgi-bin
src/cf.data.pre:7533:   SNMP responses, cachemgr.cgi output, squidclient User-Agent request header
tools/purge/purge.1:288:.if !'po4a'hide' .BR cachemgr.cgi "(8)"

@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Oct 25, 2023
@kinkie kinkie mentioned this pull request Oct 25, 2023
Closed
@yadij yadij dismissed stale reviews from rousskov and kinkie via 64f2af0 October 25, 2023 20:36
@yadij
Copy link
Contributor Author

yadij commented Oct 25, 2023

Thank you very much for removing this tool!

If this change should be reflected in doc/release-notes/release-7.sgml.in, please adjust accordingly. FWIW, recent commit 80cba6d did adjust those release notes when removing squidclient.

Done in 64f2af0

If possible, please adjust the following three leftovers as well:

INSTALL:27:the tools/cachemgr.cgi program into your httpd server's cgi-bin
src/cf.data.pre:7533:   SNMP responses, cachemgr.cgi output, squidclient User-Agent request header

Done in 90bc05c

tools/purge/purge.1:288:.if !'po4a'hide' .BR cachemgr.cgi "(8)"

This one is left intentionally to avoid conflicts with PR #1541

@yadij yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Oct 25, 2023
@yadij yadij requested a review from rousskov October 25, 2023 20:38
rousskov
rousskov previously approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

I do not insist on any changes.

doc/release-notes/release-7.sgml.in Outdated Show resolved Hide resolved
doc/release-notes/release-7.sgml.in Outdated Show resolved Hide resolved
doc/release-notes/release-7.sgml.in Outdated Show resolved Hide resolved
@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Oct 25, 2023
@yadij yadij added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-author author action is expected (and usually required) labels Oct 25, 2023
@yadij yadij requested review from kinkie and rousskov October 25, 2023 22:37
@yadij yadij added the S-could-use-an-approval An approval may speed this PR merger (but is not required) label Oct 25, 2023
rousskov
rousskov previously approved these changes Oct 25, 2023
View reviewed changes
@rousskov rousskov removed the request for review from kinkie October 25, 2023 22:48
kinkie
kinkie previously approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@kinkie kinkie left a comment

Choose a reason for hiding this comment

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

ship it!

@kinkie kinkie removed the S-could-use-an-approval An approval may speed this PR merger (but is not required) label Oct 26, 2023
@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Oct 26, 2023
@yadij yadij dismissed stale reviews from kinkie and rousskov via 737c3b7 October 27, 2023 01:43
@yadij yadij removed the S-waiting-for-author author action is expected (and usually required) label Oct 27, 2023
@yadij yadij requested a review from rousskov October 27, 2023 01:43
@yadij yadij requested review from kinkie and rousskov October 29, 2023 13:22
rousskov
rousskov reviewed Oct 29, 2023
View reviewed changes
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

I am not sure what exactly went wrong because forced push erased breadcrumbs, but it looks like that recent force-push broke this PR. FWIW, please keep in mind that forced pushes to PR branches are very rarely necessary and, in most other cases, create more problems than they solve.

@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Oct 29, 2023
@rousskov
Copy link
Contributor

I'm worried that applying the changes will invalidate the approvals

Yes, it would have invalidated the approvals, but, in this particular case, those invalidations would not cause any additional delays because this PR is authored by Amos (so his implicit approval does not get invalidated by changes), the changes would be committed by you (so you would, presumably, immediately re-approve), and the PR is more than two days old (so my/third approval would not speed things up). If it were your PR, I would have committed these trivial changes myself (instead of posting change requests) under to our "Easier Done" agreement, but I do not have a similar permission from Amos.

FWIW, this invalidation is a GitHub configuration option. We can turn it off, but I think it is the necessary evil, especially for PRs coming from folks that are not intimate with git (and may accidentally push unwanted changes that may get merged automatically). There are other solutions to mitigate that risk, but, AFAICT, they all require development and they all make the process more complex by adding more exceptions or special rules.

@yadij
Copy link
Contributor Author

yadij commented Oct 29, 2023

Had to rebase manually due to conflicts.

@rousskov
Copy link
Contributor

Had to rebase manually due to conflicts.

Resolving merge conflicts does not require rebasing.

rousskov
rousskov approved these changes Oct 29, 2023
View reviewed changes
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Approving without a careful review under the assumption that there were no material charges since the last review.

@rousskov rousskov removed the S-waiting-for-author author action is expected (and usually required) label Oct 29, 2023
@yadij yadij removed the request for review from kinkie October 29, 2023 16:09
@rousskov rousskov added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Oct 29, 2023
kinkie
kinkie approved these changes Oct 29, 2023
View reviewed changes
Copy link
Contributor

@kinkie kinkie left a comment

Choose a reason for hiding this comment

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

Expediting along. I’m confident any issues would have been caught by CI

@rousskov rousskov removed the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Oct 29, 2023
squid-anubis pushed a commit that referenced this pull request Oct 29, 2023
@yadij @squid-anubis
Remove cachemgr.cgi tool (#1542)
d554eaa
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Oct 29, 2023
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 29, 2023
@yadij yadij deleted the rm-cachemgr-cgi branch November 5, 2023 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kinkie kinkie kinkie approved these changes

@rousskov rousskov rousskov approved these changes

Assignees
No one assigned
Labels
feature maintainer needs documentation updates for merge M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yadij @kinkie @rousskov @squid-anubis