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

Prep for v7.0.1 #1988

Closed
wants to merge 8 commits into from
Closed

Prep for v7.0.1 #1988

wants to merge 8 commits into from

Conversation

kinkie
Copy link
Contributor

@kinkie kinkie commented Jan 30, 2025

Add v7.0.1 ChangeLog section.
Incorporate changes from v6.8 to v6.13 (inclusive).

@yadij

This comment was marked as resolved.

@kinkie

This comment was marked as resolved.

ChangeLog Outdated
Comment on lines 127 to 128
- Add AsList::quoted()
- Add AtMostOnce stream manipulator
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, these two and many other list items about code refactoring changes either do not belong to this list at all or need to be rephrased to become relevant to the targeted audience(s). On the other hand, I am not sure we have consensus regarding what audiences our ChangeLog should target.

Suggested change
- Add AsList::quoted()
- Add AtMostOnce stream manipulator

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for merging the suggested change. Just to avoid misunderstanding: This change request cannot be fully addressed by merging that suggestion alone because it applies to more than two entries removed by the suggested changes. That is why I used bold font for "and many other list items" text in the original request. This request is still relevant (as of recent branch commit 9cb207b).

@@ -1,3 +1,270 @@
Changes in squid-7.0.1 (2 Feb 2025):
Copy link
Contributor

Choose a reason for hiding this comment

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

Amos: Before this goes in ...

If necessary to avoid delaying v7 branching, I think we can make ChangeLog contents (including its formatting) decisions after "this goes in". In other words, we should not block v7 branching on making those belated decisions. We can change the latest ChangeLog section or even the entire ChangeLog after v7 branching and after releasing v7.0.1.

Amos: ... I think we should vote on whether v7 is going to start using the git log --oneline format for changes.

IMO, before we vote on how individual entries are formatted, we should try to agree on what ChangeLog entries should contain. For example:

  • If we agree that there should be one entry for nearly every in-scope git commit, then using git log --oneline1 becomes a natural side effect of that agreement.
  • On the other hand, if we agree that that ChangeLog entry should tell Squid admins something, then git log --oneline formatting is not going to work well because many entries would correspond to multiple git commits and, hence, have multiple relevant commit SHAs. Besides, most admins should not care about commit SHAs.

I propose the following decision making algorithm:

  • If we agree that ChangeLog primary target are Squid administrators, then we should maximize admin satisfaction: ChangeLog should be maintained/updated just like release-notes are -- hand-written items added or edited whenever admin-relevant changes are made. That is a lot more work for us (compared to what we do today), but it provides a better outcome for admins. Including commit SHAs in such entries would require post-processing (even more work for us!), but should not be needed because most admins do not work with git commit SHAs.
  • Otherwise (i.e. if we do not agree that ChangeLog primary target are Squid administrators), then we should minimize our overheads (instead of maximizing admin satisfaction): Remove ChangeLog from the repository. Generate it when packaging bootstrapped tarballs from tagged releases using git log --oneline. In the transition period (i.e. until ChangeLog generation is automated), keep ChangeLog in the repository and update it using PRs like this PR, but generate new entries using git log --oneline.

My weak preference is for the latter (i.e. generate ChangeLog) because we currently do not have enough resources to do the former (i.e. curate ChangeLog) well and should focus our scarce resources on optimizing more important (to admins) areas instead.

Footnotes

  1. Throughout my comment, I use git log --oneline to mean something like "Use formatting similar to what git log --oneline would produce, so that all or most ChangeLog entries have relevant commit SHAs. The final formatting should still be compatible with whatever (external) requirements we want to honor for ChangeLog entries. For example, we have scripts that use ChangeLog entries when posting GitHub releases. GNU, Debian, or others may have some relevant ChangeLog formatting requirements as well. Saying git log --oneline does not imply that we are going to use raw git log output and blindly violate those requirements."

Copy link
Contributor

@rousskov rousskov Jan 31, 2025

Choose a reason for hiding this comment

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

Francesco: Is there any concern with [starting with git log --oneline and then removing commit SHAs and including admin-irrelevant and obsolete entries]?

Yes, there are at least two concerns: This manual process is not cheap, especially because its first iteration usually produces unsatisfactory results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Manual process took me ~2hrs for the major release and >5 mins for a minor.
Could be better but it's not terrible

Copy link
Contributor

Choose a reason for hiding this comment

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

Manual process took me ~2hrs for the major release

That process is not over yet, and you are not the only one working on this PR.

and >5 mins for a minor

For many minor releases, that manual process required manual intervention of others as well.

Could be better but it's not terrible

Agreed, but we should aim higher than (i.e. we should not settle for) "not terrible" IMHO, especially if there is a good chance to improve something long-term without a lot of additional short-term effort. That is why I support discussing the issue raised by Amos -- there is hope we can improve things as the result of that discussion.

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.

In this and my previous review, I do not insist on any changes.

@@ -1,3 +1,270 @@
Changes in squid-7.0.1 (2 Feb 2025):
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider removing this v7 section from this master-based PR. Add it to v7 branch instead (in a different/dedicated PR created after branching v7). In that case, this PR would be focused on adding recent v6 sections and become a simpler "let's sync ChangeLog with recent v6 changes before branching v7" PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

You seem to be overlooking the "master" branch is based on the the v7 branch after the changes being documented here. These changes being documented are not exclusive to the v7.* branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just taking the pragmatic approach that a branching point is a good moment to sync things up. If at all possible, I'd like to release 7.0.1 tomorrow

Copy link
Contributor

Choose a reason for hiding this comment

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

Amos: You seem to be overlooking the "master" branch is based on the the v7 branch after the changes being documented here. These changes being documented are not exclusive to the v7.* branch.

IMO, the above statements are irrelevant to this specific suggestion and this PR in general: This ChangeLog section describes (or should describe) the diff between two commits/snapshots/releases: v6.13 and v7.0.1. It does not matter whether the underlying code is also (or will also be) present in any branches or any other commits.

For example, after the following hypothetical sequence of events:

* Released v6.13
+ Added feature A to master/v7.
+ Added feature B to master/v7.
- Removed feature A from master/v7.
* Branched v7 and released v7.0.1.

This section should look like this:

- Added feature B.

Obviously, we should copy vN ChangeLog entries to master from time to time or at some point. That point does not have to be this PR or "now". This specific suggestion was aimed at making steady progress by separating two substantially different (on several levels) changes: copying v6 entries and adding v7.0.1 section.

Francesco: I'm just taking the pragmatic approach that a branching point is a good moment to sync things up

I agree that it is a good moment to sync things up. In some sense, my approach is even more pragmatic: Limit this master PR to non-controversial copying/syncing changes that can be approved and merged into master "immediately". Move "creative" changes to a v7 PR that you can quickly merge (when needed) into v7 without waiting for approvals. It is just a suggestion though. Your call.

- Fix marking of problematic cached IP addresses
- Improved portability to MacOS
- ... and some documentation improvements

Changes in squid-6.7 (4 Feb 2024)

- Bug 5337: workaround for crash on startup if -a option is used
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider adding v5.10 section as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no SQUID_5_10 tag in the repository :\

Copy link
Contributor

@rousskov rousskov Feb 1, 2025

Choose a reason for hiding this comment

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

... but there is v5.10 ChangeLog section in v5 branch. That is enough as far as this PR is concerned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the ChangeLog session but we don't have a release. We need to identify the commit to hang it from

Copy link
Contributor

Choose a reason for hiding this comment

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

Added the ChangeLog session

FYI: This PR still does not have a v5.10 section added. The last pushed commit is 9cb207b.

but we don't have a release. We need to identify the commit to hang it from

I suggest commit a254dfe, but it is not my call to make. This additional v5.10 work can be done later, of course.

kinkie and others added 3 commits January 31, 2025 16:05
* remove entries covered by "and many code cleanups"
* group helper/feature entries together
* shuffle a few other lines for better grouping (eg a "Remove")
@rousskov
Copy link
Contributor

rousskov commented Feb 1, 2025

I polished PR description, primarily to fix spelling and versions.

@kinkie
Copy link
Contributor Author

kinkie commented Feb 1, 2025

I polished PR description, primarily to fix spelling and versions.

great, thanks!
As it seems that there are no improvements being suggested for the actual text of the ChangeLog anymore, would you mind approving the PR? @yadij @rousskov
The discussion on the process can wait

rousskov
rousskov previously approved these changes Feb 2, 2025
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.

As it seems that there are no improvements being suggested for the actual text of the ChangeLog anymore

See #1988 (comment) for a still unresolved change request.

would you mind approving the PR?

I am reluctantly approving this PR despite its unsatisfactory (IMO) shape. This unnecessary "forced" (for the lack of a better word) approvals is one of the reasons I recommended a plan that avoids them.

The discussion on the process can wait

I agree.

squid-anubis pushed a commit that referenced this pull request Feb 2, 2025
Add v7.0.1 ChangeLog section.
Incorporate changes from v6.8 to v6.13 (inclusive).
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Feb 2, 2025
@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 Feb 2, 2025
@kinkie
Copy link
Contributor Author

kinkie commented Feb 2, 2025

As it seems that there are no improvements being suggested for the actual text of the ChangeLog anymore

See #1988 (comment) for a still unresolved change request.

would you mind approving the PR?

I am reluctantly approving this PR despite its unsatisfactory (IMO) shape. This unnecessary "forced" (for the lack of a better word) approvals is one of the reasons I recommended a plan that avoids them.

Thanks for your understanding.
I see this as a one-off event for branching, mostly for convenience.
We can always iterate and improve.

Of course, adding the v5.10 ChangeLog means I voided your formal approval, hopefully this can be redone by EOD.

squid-anubis pushed a commit that referenced this pull request Feb 2, 2025
Add v7.0.1 ChangeLog section.
Incorporate changes from v6.8 to v6.13 (inclusive).
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-failed-staging-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-abandoned-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Feb 2, 2025
@rousskov
Copy link
Contributor

rousskov commented Feb 2, 2025 via email

@kinkie kinkie removed the M-failed-staging-other https://github.com/measurement-factory/anubis#pull-request-labels label Feb 2, 2025
@kinkie kinkie added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels backport-to-v7 maintainer has approved these changes for v7 backporting labels Feb 2, 2025
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Feb 2, 2025
kinkie added a commit to kinkie/squid that referenced this pull request Feb 2, 2025
Add v7.0.1 ChangeLog section.
Incorporate changes from v6.8 to v6.13 (inclusive).
kinkie added a commit that referenced this pull request Feb 2, 2025
Add v7.0.1 ChangeLog section.
Incorporate changes from v6.8 to v6.13 (inclusive).
kinkie added a commit to kinkie/squid that referenced this pull request Feb 26, 2025
Add v7.0.1 ChangeLog section.
Incorporate changes from v6.8 to v6.13 (inclusive).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-v7 maintainer has approved these changes for v7 backporting M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants