Aki Sasaki (not active) Assignee
	Description • 10 years ago

We currently have a number of manual wiki.m.o updates for merge day:
https://wiki.mozilla.org/ReleaseEngineering/Merge_Duty/Steps#Change_Template_Version_Numbers

It would be nice to automate them.

	Aki Sasaki (not active) Assignee
	Comment 1 • 10 years ago

Asked in #webdev; will record response in here if any.

	Aki Sasaki (not active) Assignee
	Comment 2 • 10 years ago

http://hg.mozilla.org/build/tools/file/aefd18f890da/buildfarm/maintenance/update_maintenance_wiki.sh !

	Aki Sasaki (not active) Assignee
	Comment 3 • 10 years ago

This refactors update_maintenance_wiki.sh quite a bit.  It doesn't remove all redundancy, but it gets rid of major parts of it.
I should probably do a pass of tests on that after copying the maintenance wiki to my user page.

	Aki Sasaki (not active) Assignee
	Comment 4 • 10 years ago

Oops, switch to production configs.

	Pete Moore [:pmoore][:pete]
	Comment 5 • 10 years ago

Comment on attachment 8454224 [details] [diff] [review]
wiki_bump

Review of attachment 8454224 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Aki,

Overall this looks great! I appreciate that you factored out the common logic, rather than duplicating, and I think it is now much better.

I haven't tested update_merge_day_wiki.sh - I guess you will be testing it the first times you use it, since you will no doubt be calling it manually for the first few iterations, to check all is ok. Were you able to test it much already?

There are a couple of key fixes which mean I have to r-, for example I believe update_maintenance_wiki.sh would now be broken due to the line with a variable referenced inside single quotes, which needs to be double quotes. Most of my comments are just stylistic suggestions, rather than hard requirements - I tried to write when I prefer something to when I would block on it.

Looking forward to seeing it in production. :)

Pete

::: buildfarm/maintenance/update_maintenance_wiki.sh
@@ +4,5 @@
>  unset DRY_RUN DEBUG_DIR
>  WIKI_TEXT_ADDITIONS_FILE=""
>  
> +cwd=$(cd $(dirname $0); pwd)
> +. ${cwd}/wiki_functions.sh

This doesn't work when the user has a space in a parent directory name. 

pmoore@Elisandra:~/Work Repos/tools/buildfarm/maintenance bug1036573 $ ./update_maintenance_wiki.sh -d
./update_maintenance_wiki.sh: line 8: .: /Users/pmoore/Work: is a directory
pmoore@Elisandra:~/Work Repos/tools/buildfarm/maintenance bug1036573 $ 


To fix, use 3 sets of double quotes:

cwd=$(cd "$(dirname "$0")"; pwd)
. "${cwd}/wiki_functions.sh"

Alternatively, combine the two lines into one (my preferred option):

. "$(dirname "${0}")/wiki_functions.sh"

@@ +17,5 @@
>      echo "    -d:                           Dry run; will not make changes, only validates login."
>      echo "    -h:                           Display help."
>      echo "    -r DEBUG_DIR:                 Copy temporary generated files into directory DEBUG_DIR"
>      echo "    -w WIKI_TEXT_ADDITIONS_FILE:  File containing wiki markdown to insert into wiki page."
> +    echo "You need to set WIKI_USERNAME and WIKI_PASSWORD in env before running."

nice catch!

@@ +84,5 @@
>          exit 72
>      fi
>  fi
>  
> +check_wiki_login_env ${WIKI_TITLE}

nit: quotes preferred for stylistic reasons (the function takes one parameter, so putting quotes around ${WIKI_TITLE} ensures only one parameter is passed - even though in this case ${WIKI_TITLE} has no spaces)

@@ +93,3 @@
>  
> +echo "  * Retrieving current wiki text of https://wiki.mozilla.org/${WIKI_TITLE}..."
> +curl -s 'https://wiki.mozilla.org/${WIKI_TITLE}&action=raw' >> "${current_content}"

This needs to be double quotes as it contains a variable, i.e.:

curl -s "https://wiki.mozilla.org/${WIKI_TITLE}&action=raw" >> "${current_content}"

::: buildfarm/maintenance/update_merge_day_wiki.sh
@@ +13,5 @@
> +#SIX_WEEK_DATE_TITLES="User:Asasaki:TestReleaseDate"
> +WIKI_COMMENT="Merge day"
> +
> +cwd=$(cd $(dirname $0); pwd)
> +. ${cwd}/wiki_functions.sh

see my comments for update_maintenance_wiki.sh about these two lines above

@@ +22,5 @@
>      echo
> +    echo "    -h:                    Display help."
> +    echo "    -b B2G_VERSION:        REQUIRED! New B2G version for mozilla-central. e.g. 2.1"
> +    echo "    -e ESR_VERSION:        New ESR version (only set this when a new ESR version comes along!)"
> +    echo "    -r NEXT_RELEASE_DATE:  Next release date YYYY-MM-DD . By default we'll increment 6 weeks."

nit: space after YYYY-MM-DD

@@ +96,3 @@
>      fi
> +    echo ${B2G_VERSION} > "${new_content}"
> +    wiki_edit_login

do you need to call wiki_edit_login for every edit, or can one edit token be used for multiple postings? i'm not sure myself. might be worth testing putting this outside the loop, and seeing if it still works.

@@ +117,5 @@
> +            rm "${current_content}"
> +            continue
> +        fi
> +        echo ${ESR_VERSION} > "${new_content}"
> +        wiki_edit_login

same here

@@ +157,1 @@
>  

These three loops seem almost identical - is it worth putting them in a function, and then calling them with different parameters?

::: buildfarm/maintenance/wiki_functions.sh
@@ +1,1 @@
>  #!/bin/bash -e

This line has no effect and can be removed. It only has an effect if you execute a script, not if you source it. If you want to ensure -e option, you could write this instead as:

set -e

Although this will affect the environment of the script this file is sourced into, which may not be desirable. I'd be tempted to leave it out altogether.

Example:

pmoore@Elisandra:~/Work Repos/tools/buildfarm/maintenance bug1036573 $ cat test.sh 
#!/bin/bash

. "$(dirname "${0}")/demo.sh"
pmoore@Elisandra:~/Work Repos/tools/buildfarm/maintenance bug1036573 $ cat demo.sh 
#!/bin/bash -e
# the above line has no effect, when this file is sourced, so will *not* exit after this command fails

false
echo still running

# now use set -e instead

set -e
false

# now it has exited
echo script does not reach this point

pmoore@Elisandra:~/Work Repos/tools/buildfarm/maintenance bug1036573 $ ./test.sh 
still running
pmoore@Elisandra:~/Work Repos/tools/buildfarm/maintenance bug1036573 $

@@ +4,5 @@
> +    if [ $#  -gt 0 ] ; then
> +        t=$1
> +    else
> +        t=""
> +    fi

t="${1:-}" instead of if block?

@@ +26,4 @@
>  }
>  
> +function wiki_login {
> +    # Requires WIKI_TITLE set; maybe we should pass this in as an arg

I think it would be nicer for all these functions if they took parameters, and if there was a comment for each of them, to say which parameters they take. I won't block on this, but I think it would be a nice improvement.

@@ +45,3 @@
>  }
>  
> +function wiki_edit_login {

it would be useful to have a comment about which global variables are required, but i think i'd prefer the functions to have local variables, e.g.

local login_token="${1}"
local WIKI_TITLE="${2}"
...

and then with a comment to say what parameters need to be passed

I won't block on that, as I also see simplicity in having global variables - but i think if we go with global variables, we will need at least the function to be documented about which variables need to be set, e.g. as a comment.

@@ +81,4 @@
>  
> +function wiki_post {
> +    # now post new content...
> +    # Requires WIKI_TITLE, WIKI_COMMENT, new_content set; maybe we should pass these in as args

Agreed - think it might be nicer to pass them in too, but won't block on it.

	Pete Moore [:pmoore][:pete]
	Comment 6 • 10 years ago

> > +    wiki_edit_login
> 
> do you need to call wiki_edit_login for every edit, or can one edit token be
> used for multiple postings? i'm not sure myself. might be worth testing
> putting this outside the loop, and seeing if it still works.

It looks like only one edit token is needed per login, rather than per page edit:
https://www.mediawiki.org/wiki/API:Edit#Token

So it looks like wiki_edit_login can be taken out of the loop in both cases. It could also be worth merging wiki_login and wiki_edit_login into one method, since I believe that we always need both, i.e. you wouldn't call one without the other, so there might be merit in having a single method to log in and get an edit token.

	Pete Moore [:pmoore][:pete]
	Comment 7 • 10 years ago

Comment on attachment 8454224 [details] [diff] [review]
wiki_bump

Review of attachment 8454224 [details] [diff] [review]:
-----------------------------------------------------------------

::: buildfarm/maintenance/update_merge_day_wiki.sh
@@ +96,3 @@
>      fi
> +    echo ${B2G_VERSION} > "${new_content}"
> +    wiki_edit_login

I take it back - I see the edit token depends on the WIKI_TITLE which changes in the loop, so it looks like I was wrong. Sorry for the noise!

@@ +117,5 @@
> +            rm "${current_content}"
> +            continue
> +        fi
> +        echo ${ESR_VERSION} > "${new_content}"
> +        wiki_edit_login

and i take it back again here :)

	Aki Sasaki (not active) Assignee
	Comment 9 • 10 years ago

(In reply to Pete Moore [:pete][:pmoore] from comment #5)
> I haven't tested update_merge_day_wiki.sh - I guess you will be testing it
> the first times you use it, since you will no doubt be calling it manually
> for the first few iterations, to check all is ok. Were you able to test it
> much already?

Yeah, that's why I had the User:Asasaki titles at the top for the WIKI_TITLEs.

> Alternatively, combine the two lines into one (my preferred option):
>
> . "$(dirname "${0}")/wiki_functions.sh"

Done, both files.

> @@ +84,5 @@
> >          exit 72
> >      fi
> >  fi
> >
> > +check_wiki_login_env ${WIKI_TITLE}
>
> nit: quotes preferred for stylistic reasons (the function takes one
> parameter, so putting quotes around ${WIKI_TITLE} ensures only one parameter
> is passed - even though in this case ${WIKI_TITLE} has no spaces)

Fixed.

>
> @@ +93,3 @@
> >
> > +echo "  * Retrieving current wiki text of https://wiki.mozilla.org/${WIKI_TITLE}..."
> > +curl -s 'https://wiki.mozilla.org/${WIKI_TITLE}&action=raw' >> "${current_content}"
>
> This needs to be double quotes as it contains a variable, i.e.:
>
> curl -s "https://wiki.mozilla.org/${WIKI_TITLE}&action=raw" >>
> "${current_content}"

Fixed.

> @@ +22,5 @@
> >      echo
> > +    echo "    -h:                    Display help."
> > +    echo "    -b B2G_VERSION:        REQUIRED! New B2G version for mozilla-central. e.g. 2.1"
> > +    echo "    -e ESR_VERSION:        New ESR version (only set this when a new ESR version comes along!)"
> > +    echo "    -r NEXT_RELEASE_DATE:  Next release date YYYY-MM-DD . By default we'll increment 6 weeks."
>
> nit: space after YYYY-MM-DD

As mentioned in IRC, this was intentional to avoid having someone do |-r 2014-09-01.|  Ideally no one's quite that literal, but you never know.

> @@ +157,1 @@
> >
>
> These three loops seem almost identical - is it worth putting them in a
> function, and then calling them with different parameters?

I thought about that, but they're different enough that it would either be less of a win or overly complex.  Also, I'm hoping this is a write once, never touch again script.  Together it made me decide not to unless we end up needing to do another loop or set of loops later.

> ::: buildfarm/maintenance/wiki_functions.sh
> @@ +1,1 @@
> >  #!/bin/bash -e
>
> This line has no effect and can be removed. It only has an effect if you
> execute a script, not if you source it.

Done.

> @@ +4,5 @@
> > +    if [ $#  -gt 0 ] ; then
> > +        t=$1
> > +    else
> > +        t=""
> > +    fi
>
> t="${1:-}" instead of if block?

Done.

> @@ +26,4 @@
> >  }
> >
> > +function wiki_login {
> > +    # Requires WIKI_TITLE set; maybe we should pass this in as an arg
>
> I think it would be nicer for all these functions if they took parameters,
> and if there was a comment for each of them, to say which parameters they
> take. I won't block on this, but I think it would be a nice improvement.

I agree, and I think the next person who wants to use the wiki_functions.sh library can do so :)

> @@ +45,3 @@
> >  }
> >
> > +function wiki_edit_login {
>
> it would be useful to have a comment about which global variables are
> required, but i think i'd prefer the functions to have local variables, e.g.
>
> local login_token="${1}"
> local WIKI_TITLE="${2}"
> ...
>
> and then with a comment to say what parameters need to be passed
>
> I won't block on that, as I also see simplicity in having global variables -
> but i think if we go with global variables, we will need at least the
> function to be documented about which variables need to be set, e.g. as a
> comment.

Added the comment, but it missed the interdiff.
+    # Requires you to have run wiki_login already, and also set WIKI_TITLE.

(In reply to Pete Moore [:pete][:pmoore] from comment #7)
> ::: buildfarm/maintenance/update_merge_day_wiki.sh
> @@ +96,3 @@
> >      fi
> > +    echo ${B2G_VERSION} > "${new_content}"
> > +    wiki_edit_login
>
> I take it back - I see the edit token depends on the WIKI_TITLE which
> changes in the loop, so it looks like I was wrong. Sorry for the noise!

Yup.  I think there's a way to login to multiple titles simultaneously, but it seemed like it might get complex to gather all the titles at the beginning and then loop.

	Pete Moore [:pmoore][:pete]
	Comment 10 • 10 years ago

Comment on attachment 8454599 [details] [diff] [review]
wiki_bump

Review of attachment 8454599 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Aki. Looks good.

	Aki Sasaki (not active) Assignee
	Comment 11 • 10 years ago

Comment on attachment 8454599 [details] [diff] [review]
wiki_bump

http://hg.mozilla.org/build/tools/rev/a52cd8b0f1e7

Todo: docs

	Aki Sasaki (not active) Assignee
	Comment 12 • 10 years ago

Docs: https://wiki.mozilla.org/index.php?title=ReleaseEngineering/Merge_Duty/Steps&diff=996680&oldid=996151