Skip to content

Fix cfgman documentation release #37

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

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

Fix cfgman documentation release #37

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions release/mk-cfgman-docs.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,25 @@

. ~/.server.config || exit $?

# Known Bug:
# Does not remove references to directives dropped. This is
# an issue for branch 'master' directive erasure.
Comment on lines -9 to -11
Copy link
Contributor

@rousskov rousskov Sep 25, 2025

Choose a reason for hiding this comment

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

Is not this bug still present? AFAICT, the DYN part of the loop iterates previously published *.html directive files and does not erase any stale ones.

# clean the current workspace
gitCleanWorkspace ()
{
git clean --quiet -xdf --exclude="\.BASE"
git checkout --quiet -- .
Copy link
Contributor

@rousskov rousskov Sep 25, 2025

Choose a reason for hiding this comment

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

A checkout command does not belong to a "clean workspace" function. The command itself looks a bit odd.

If you want to erase changes to tracked files, then use something like git reset --hard. If tracked files may remain as is, then remove this command (from this function). Let the caller checkout what they need (one caller already does, resulting in two sequential checkouts).

}

for version in `ls -1 $SQUID_VCS_PATH | grep squid | cut -d- -f2`; do

! test -d $SQUID_WWW_PATH/content/Versions/$version/cfgman && continue
# Update the website /Versions/v*/cfgman/ HTML documents
cd $SQUID_VCS_PATH/squid-$version || continue
gitCleanWorkspace
git checkout v$version &&
Copy link
Contributor

@rousskov rousskov Sep 25, 2025

Choose a reason for hiding this comment

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

If this script relies on something else fetching changes from the remote repository, I recommend adding a comment about it. Otherwise, it needs to do something like "git fetch".

./bootstrap.sh && ./configure && make -C ./doc cfgman &&
mv -f -t $SQUID_WWW_PATH/content/Versions/$version/cfgman ./doc/cfgman/*
Comment on lines +21 to +23
Copy link
Contributor

@rousskov rousskov Sep 25, 2025

Choose a reason for hiding this comment

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

AFAICT, this particular command sequence ignores failures and lets the current iteration proceed to the "update DYN documents" step that, AFAICT, depends on the failed command. If we do not want to abort the script on failures, should not we proceed to the next iteration instead?

gitCleanWorkspace

# Update the website /Doc/config/ DYN documents
Copy link
Contributor

@rousskov rousskov Sep 25, 2025

Choose a reason for hiding this comment

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

Does this update use some make-generated files? If yes, we should not clean the working directory before this update (and we should not update if make fails). If not, I recommend rephrasing this comment to clarify where the new information is coming from, to remove the current implication that this information is the result of the above make.

! test -d $SQUID_WWW_PATH/content/Versions/$version/cfgman && continue
Copy link
Contributor

@rousskov rousskov Sep 25, 2025

Choose a reason for hiding this comment

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

It is strange to check whether this directory is present when the above commands rely on its presence to store their result. Should not this test be moved higher, where it used to reside before this PR?

for directive in $SQUID_WWW_PATH/content/Versions/$version/cfgman/*.html; do
directive=`basename $directive .html`

Expand Down