-
Notifications
You must be signed in to change notification settings - Fork 42
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
Parsing the WMA_TAG and using only the release part for creating the WMA_BUILD_ID #1485
Parsing the WMA_TAG and using only the release part for creating the WMA_BUILD_ID #1485
Conversation
Not to copy paste the test results in multiple places, I am just linking them in the current comment here from the WMcore issue: dmwm/WMCore#11990 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@todor-ivanov changes are looking good to me. However, I have made suggestion for minor improvements.
docker/pypi/wmagent/install.sh
Outdated
echo | ||
echo "=======================================================================" | ||
echo "Starting new WMAgent deployment with the following initialisation data:" | ||
echo "-----------------------------------------------------------------------" | ||
echo " - WMAgent Version : $WMA_TAG" | ||
echo " - WMAgent Tag : $WMA_TAG" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to keep it saying "WMAgent Version".
Then rephrase "WMAgent Release" to "WMAgent Release Cycle".
Lastly, remove the Major, Minor and Patch, and those might be confusing and maybe even incorrect (e.g., for release 2.3.3).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok,
... the way I dd it was to be extra explicit in the meaning and the matching between the print outs and the variables themselves, but that's minor. What I cannot understand is why would the major/minor
and patch
part of the tag be incorrect for a tag like WMA_TAG=2.3.3
? I have tested it many times and in the case of a tag like this what those pirntouts would say would be:
#11 0.115 =======================================================================
#11 0.115 Starting new WMAgent deployment with the following initialisation data:
#11 0.115 -----------------------------------------------------------------------
#11 0.116 - WMAgent Tag : 2.3.3
#11 0.116 - WMAgent Release : 2.3.3
#11 0.116 - WMAgent Major version : 2.3
#11 0.116 - WMAgent Minor version : 3
#11 0.116 - WMAgent Patch/Release cand :
#11 0.116 - WMAgent User :
#11 0.116 - WMAgent Root path : /data
#11 0.118 - Python Version : Python 3.8.16
#11 0.118 - Python Module path : /usr/local/lib/python3.8/site-packages
#11 0.118 =======================================================================
Which is exactly what is expected I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the concept of release type for our tagging convention is not straight forward. For instance, for 2.3.4, I would say:
- major release: 2
- middle release: 3
- minor release: 4
but for 2.3.4.5, I see two ways to read it. Either as:
- major release: 2
- middle release: 3
- minor release: 4
- patch release: 5
OR - major release: 2
- middle release: 3.4
- minor release: 5
- patch release:
Due to the potential confusion on this interpretation, I feel like it is better not to give such information.
docker/pypi/wmagent/install.sh
Outdated
@@ -105,7 +116,8 @@ echo "-----------------------------------------------------------------------" | |||
stepMsg="Generating and preserving current build id" | |||
echo "-----------------------------------------------------------------------" | |||
echo "Start $stepMsg" | |||
echo $RANDOM |sha256sum |awk '{print $1}' > $WMA_ROOT_DIR/.dockerBuildId | |||
|
|||
echo ${WMA_VER[release]}|sha256sum |awk '{print $1}' > $WMA_ROOT_DIR/.dockerBuildId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make it more readable, I would suggest whitespaces around the pipe operation.
docker/pypi/wmagent/install.sh
Outdated
# Parsing the WMA_TAG | ||
declare -A WMA_VER | ||
WMA_VER[release]=$(echo $WMA_TAG|sed -nr 's/.*(^[0-9]+\.[0-9]+\.[0-9]+).*$/\1/p') | ||
WMA_VER[patch]=$(echo $WMA_TAG|sed -nr 's/.*(^[0-9]+\.[0-9]+\.[0-9]+)//p') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make it more readable, I would suggest whitespaces around the pipe operation. Same for the line above.
7aa7440
to
a0a069f
Compare
…WMA_BUILD_ID Review comments Forgotten space
a0a069f
to
7f27b0e
Compare
hi @amaltaro addressed your comments. could you please take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Todor. You missed the whitespaces around |
operator, but we can move forward as is.
Fixes: dmwm/WMCore#11990
With the current PR we create the WMA_BUILD_ID based only on the release part of the WMA_TAG of the currently built agent. This way we should avoid restarting initialization process (hence agent data and databse deletion) on patch releases or release candidates.