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

Add new required branches on series release #22

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

yadij
Copy link
Contributor

@yadij yadij commented Mar 25, 2023

vN-bootstrap and vN-next-backports are now also required.

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.

What are we using vN-bootstrap branches for? If each such branch is used to store bootstrapped Squid vN sources, then why do we want them stored in a git branch?

@yadij
Copy link
Contributor Author

yadij commented Mar 30, 2023

Some QA person who wishes to remain unnamed required that we submit each automation change as a separate PR for merge to master. That imposes side effects - one of which is a need to track which automation is doing which changes. That branch isolates any changes from bootstrap.sh[1] automation away from other automation.

[1] should only be updating documentation files now.

@rousskov
Copy link
Contributor

That branch isolates any changes from bootstrap.sh[1] automation away from other automation.

[1] should only be updating documentation files now.

What are those "bootstrap.sh automation" (documentation) changes? Perhaps you can point me to the script that does them or an already merged commit dedicated to them? I cannot quickly search the repositories right now and am just trying to understand what they are...

@yadij
Copy link
Contributor Author

yadij commented Mar 31, 2023

That branch isolates any changes from bootstrap.sh[1] automation away from other automation.
[1] should only be updating documentation files now.

What are those "bootstrap.sh automation" (documentation) changes?

https://github.com/squid-cache/squid/blob/master/bootstrap.sh#L154

Perhaps you can point me to the script that does them or an already merged commit dedicated to them?

https://github.com/squid-cache/ci/blob/main/maintenance/code-maintenance.sh#L90

@@ -66,8 +70,12 @@ else
ln -s $SQUID_VCS_PATH/gitignore $SQUID_VCS_PATH/squid-$NEWVER/.git/info/exclude

echo " .. Creating maintenance branches ..."
git checkout -b v${NEWVER}-bootstrap &&
git push -u origin v${NEWVER}-bootstrap
Copy link
Contributor

@rousskov rousskov Apr 3, 2023

Choose a reason for hiding this comment

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

That branch isolates any changes from bootstrap.sh[1] automation away from other automation.
[1] should only be updating documentation files now.

What are those "bootstrap.sh automation" (documentation) changes?

https://github.com/squid-cache/squid/blob/master/bootstrap.sh#L154

That reference shows that squid/bootstrap.sh creates a SPONSORS file. AFAICT, that file is not versioned since 2012 commit squid-cache/squid@bdb40e3. The same bootstrap.sh script creates a lot of other unversioned files (e.g., ./configure), of course. I see no command that adds SPONSORS to some git branch. Did I miss it?

Perhaps you can point me to the script that does them or an already merged commit dedicated to them?

https://github.com/squid-cache/ci/blob/main/maintenance/code-maintenance.sh#L90

AFAICT, that runMaintenanceScript ./bootstrap.sh command will never result in a PR because it/bootstrap.sh never modifies any git-versioned files and runMaintenanceScript() does not add any unversioned files to the branch. AFAICT, that command has no positive effect and should be removed, along with any code related to the vN-bootstrap branches (instead of adding handling for vN-bootstrap branches to this script). Am I missing something?

I understand that bootstrap.sh should be executed at some point to make bootstrapped tarballs, but this PR/discussion is not about bootstrapped tarball creation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This branch is protecting any changes which the bootstrap automation stage might be performing (now or in future). Preventing any versioned source changes from being performed then silently erased by or prior to the next step of automation.

Copy link
Contributor

@rousskov rousskov May 16, 2023

Choose a reason for hiding this comment

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

This branch is protecting any changes which the bootstrap automation stage might be performing (now or in future).

Bootstrap.sh, by design, should not perform any relevant changes. AFAICT, bootstrap.sh does not perform any relevant changes now. At the very least, no such changes are known. Bootstrap.sh will not perform any relevant changes in the future (because any bootstrap.sh modifications that add them will be blocked as buggy/misplaced).

Preventing any versioned source changes from being performed then silently erased by or prior to the next step of automation.

If we really want to automate detecting unwanted bootstrap.sh side effects, we can do that, but creating a dedicated branch with corresponding public PRs is not the right approach even for that task.

@yadij yadij requested a review from rousskov May 16, 2023 03:32
@yadij yadij added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label May 16, 2023
@rousskov rousskov removed their request for review May 16, 2023 13:33
@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 May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-for-author author action is expected (and usually required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants