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

ext_time_quota_acl: remove -l option #1909

Merged
merged 4 commits into from
Oct 11, 2024
Merged

Conversation

kinkie
Copy link
Contributor

@kinkie kinkie commented Oct 2, 2024

Supporting logging to a file complicates upgrading helper code to use
debugs() because DebugFile code calls commSetCloseOnExec(), and our
comm/libminimal does not currently provide a functioning implementation
for that API: The existing implementation is an unconditional assert.
To save development time while upgrading helpers, we are dropping this
feature. It can probably be emulated using shell redirection tricks.


Manual backport of master/v7 commit 2d93cfe.

Supporting logging to a file complicates upgrading helper code to use
debugs() because DebugFile code calls commSetCloseOnExec(), and our
comm/libminimal does not currently provide a functioning implementation
for that API: The existing implementation is an unconditional assert.
To save development time while upgrading helpers, we are dropping this
feature. It can probably be emulated using shell redirection tricks.

----
Backport of PR squid-cache#1872
@kinkie
Copy link
Contributor Author

kinkie commented Oct 2, 2024

Note: after merging this backport we will need to do a followup PR to
v7 to move the release note content from the v7 to the v6 file

@squid-anubis squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Oct 2, 2024
@rousskov
Copy link
Contributor

rousskov commented Oct 2, 2024

I have adjusted PR description footer to refer to a (more permanent and actually meaningful in some contexts1) branch commit rather than GitHub PR.

@kinkie, I recommend adjusting that footer further to also mention why manual changes were necessary in this case and documenting their perceived risks, divergence from master/v7 code, etc. The reader already knows that cherry-picking failed, but does not know what the side effects of those manual changes were. Let's help the reader understand what they are dealing with!

Footnotes

  1. Imagine, for example, dealing with v6 branch a few months from now, when we have both v7 and master/v8 branches. It is often useful to know where (i.e. which branch) a particular change was backported from. The same "source branch" information can be restored using git, but it only takes three extra characters to inline it (by adding "/v7" suffix) and make it readily available to the reader... I do not insist on adding such suffixes, of course. Your call!

@rousskov
Copy link
Contributor

rousskov commented Oct 2, 2024

I also fixed PR description formatting using the original commit message.

@squid-anubis squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Oct 2, 2024
@kinkie
Copy link
Contributor Author

kinkie commented Oct 6, 2024

ok to test

1 similar comment
@kinkie
Copy link
Contributor Author

kinkie commented Oct 7, 2024

ok to test

@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 8, 2024
Copy link
Contributor

@yadij yadij left a comment

Choose a reason for hiding this comment

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

Just some polish to the release notes that was missed first time around. Not insisting on a change.

doc/release-notes/release-6.sgml.in Outdated Show resolved Hide resolved
doc/release-notes/release-6.sgml.in Outdated Show resolved Hide resolved
@kinkie kinkie added the M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels label Oct 10, 2024
squid-anubis pushed a commit that referenced this pull request Oct 10, 2024
Supporting logging to a file complicates upgrading helper code to use
debugs() because DebugFile code calls commSetCloseOnExec(), and our
comm/libminimal does not currently provide a functioning implementation
for that API: The existing implementation is an unconditional assert.
To save development time while upgrading helpers, we are dropping this
feature. It can probably be emulated using shell redirection tricks.

----
Manual backport of master/v7 commit 2d93cfe.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Oct 10, 2024
squid-anubis pushed a commit that referenced this pull request Oct 11, 2024
Supporting logging to a file complicates upgrading helper code to use
debugs() because DebugFile code calls commSetCloseOnExec(), and our
comm/libminimal does not currently provide a functioning implementation
for that API: The existing implementation is an unconditional assert.
To save development time while upgrading helpers, we are dropping this
feature. It can probably be emulated using shell redirection tricks.

----
Manual backport of master/v7 commit 2d93cfe.
@squid-anubis squid-anubis added M-abandoned-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 11, 2024
@yadij
Copy link
Contributor

yadij commented Oct 11, 2024

Jenkins failing with Java errors. Re-basing to kick a retest and see if they are gone.

@yadij yadij removed the M-abandoned-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Oct 11, 2024
@squid-anubis squid-anubis added the M-abandoned-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Oct 11, 2024
@kinkie
Copy link
Contributor Author

kinkie commented Oct 11, 2024

Jenkins failing with Java errors. Re-basing to kick a retest and see if they are gone.

yeah, it happens sometimes; it's race conditions on temporary commits I think

@yadij yadij removed M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels M-abandoned-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 11, 2024
@yadij
Copy link
Contributor

yadij commented Oct 11, 2024

FYI, I have not heard anything about Anubis being fixed to handle the vN branch merges yet. So having it test backports is a waste of resources.

@squid-anubis squid-anubis added the M-abandoned-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Oct 11, 2024
squid-anubis pushed a commit that referenced this pull request Oct 11, 2024
Supporting logging to a file complicates upgrading helper code to use
debugs() because DebugFile code calls commSetCloseOnExec(), and our
comm/libminimal does not currently provide a functioning implementation
for that API: The existing implementation is an unconditional assert.
To save development time while upgrading helpers, we are dropping this
feature. It can probably be emulated using shell redirection tricks.

----
Manual backport of master/v7 commit 2d93cfe.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-abandoned-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 11, 2024
@kinkie kinkie merged commit 819f120 into squid-cache:v6 Oct 11, 2024
5 checks passed
@rousskov
Copy link
Contributor

FYI, I have not heard anything about Anubis being fixed to handle the vN branch merges yet.

FWIW, I am not aware of any Anubis bugs in this context. IIRC, Anubis treats all target branches the same -- it does not treat the master branch specially. If you want to use Anubis for merging vN PRs and need Anubis to treat master and those branches differently, please let me know what different treatment you would like to see or file/update the corresponding issue. Maintainers are in the driving seat for these activities.

So having it test backports is a waste of resources.

Even if a PR is merged manually, testing of staged commits (created by Anubis) may be valuable: Jenkins and GitHub test PR merge commits which are often slightly different from staged commits. Besides testing, staged commits offer correct commit message (that can be edited by editing PR title/description) and correct code tree changes. Squid vN branches have bad commits (e.g., commit b4addc2, commits 5f6a24f..23b3ee0, and commit 94bdd49) that would not exist if staged commits were merged (manually or automatically).

rousskov added a commit to measurement-factory/squid that referenced this pull request Oct 29, 2024
rousskov added a commit to measurement-factory/squid that referenced this pull request Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants