Update git clone logic in build.xml #4891

annaibm · 2023-11-28T20:47:01Z

Removes prestaged jck material and reclones it, if git command fails.

resolves: #4846

annaibm · 2023-11-28T20:52:05Z

Grinder_jck test: 2010/2011

LongyuZhang

Regarding the testing, I think we can reserve a machine to delete the git folder, then mkdir a normal folder with the same name (so git will fail), in this scenario, your code will be tested then.

jck/build.xml

annaibm · 2023-12-01T18:06:03Z

Grinder_jck test: 2057

jck/build.xml

LongyuZhang

LGTM. Thanks @annaibm

llxia

This problem is not limited to git clone. It is for general git cmd. I think it would be easier to rewrite the logic from scratch. ant script may not be the best way to do it.

llxia · 2023-12-13T19:41:40Z

Converted the PR in draft. Please change the PR in Ready for review when it is ready.

annaibm · 2023-12-18T16:26:20Z

Grinder Test: 2103

jck/build.xml

LongyuZhang · 2024-01-03T20:35:11Z

jck/build.xml

-				</if>
-			</then>
-		</if> 
+		<exec executable="bash" failonerror="false">


Not sure if we need to set true here?

jck/update_git_material.sh

jck/build.xml

jck/update_git_material.sh

karianna · 2024-01-05T20:45:16Z

jck/update_git_material.sh

+    fi
+}
+
+# Function to perform hard reset for z/OS


Why is this needed?

Will need to remove this function as perform_hard_reset_and_pull needs to handle both ZOS and non-ZOS.

jck/update_git_material.sh

llxia · 2024-01-08T18:47:09Z

jck/update_git_material.sh

+    cd "$JCK_ROOT_USED" || {
+        echo "Failed to change directory to $JCK_ROOT_USED"
+        exit 1
+    }


Did you experience this case where the dir exist but you cannot cd into? The only case that I can think of is due to permission. The risk is very low. If it happens, this should be caught in line 140. I would prefer to remove L134 - L138.

jck/update_git_material.sh

llxia · 2024-01-08T19:09:14Z

The current PR only adds the error check in the places that we think it may fail. This is not sufficient. The better way to do this is to use try/catch. In shell, we mimic try/catch as follows.

{ # try

  # commands, such as check, update, git clone, etc

} || { # catch
  git_clone
}

https://stackoverflow.com/questions/22009364/is-there-a-try-catch-command-in-bash

Also, this can help us to write clean code to avoid repeatedly calling the same functions. For example, remove_and_clone is called 6 times in the PR.

llxia · 2024-01-19T18:40:58Z

jck/update_git_material.sh

+# Catch block
+if [ "$global_error" -eq 1 ]; then
+    handle_error
+fi


Did the following work?

{ check_and_handle_updates } || { git_clone }

I had tried the above method but it doesnt catch errors happening under functions within perform_hard_reset or other places as such. So need to add error handling to check the error code returned in addition to trap statement.

jck/update_git_material.sh

llxia · 2024-01-22T16:47:42Z

jck/update_git_material.sh

+
+# Try block
+if [ -d "$JCK_ROOT_USED" ] && { [ -d "$JCK_ROOT_USED/.git" ] || git -C "$JCK_ROOT_USED" rev-parse --is-inside-work-tree &>/dev/null; }; then
+	echo "The directory is a valid Git repository."


We need to clean all unstaged change.

git -C $JCK_ROOT_USED clean -fd

Yes we are handling it under check_and_handle_updates method, does that suffice?

no, it is not. In some cases, someone can manually change the file on disk without committing. Therefore, the SHA is the same. check_and_handle_updates does not catch this case.

- Removes prestaged jck material and reclones it, if git command fails resolves: adoptium#4846 Signed-off-by: Anna Babu Palathingal <[email protected]>

LongyuZhang reviewed Dec 1, 2023

View reviewed changes

jck/build.xml Outdated Show resolved Hide resolved

annaibm force-pushed the update_GitClone_4846 branch 2 times, most recently from a5c664d to de22015 Compare December 1, 2023 16:45

annaibm marked this pull request as ready for review December 1, 2023 18:08

karianna approved these changes Dec 2, 2023

View reviewed changes

jck/build.xml Outdated Show resolved Hide resolved

annaibm force-pushed the update_GitClone_4846 branch from de22015 to 1c67fbc Compare December 4, 2023 14:24

LongyuZhang approved these changes Dec 4, 2023

View reviewed changes

LongyuZhang requested a review from llxia December 4, 2023 14:26

llxia requested changes Dec 4, 2023

View reviewed changes

annaibm force-pushed the update_GitClone_4846 branch 5 times, most recently from 4d99ca5 to ad497ba Compare December 13, 2023 19:31

karianna requested a review from llxia December 13, 2023 19:33

llxia marked this pull request as draft December 13, 2023 19:40

annaibm force-pushed the update_GitClone_4846 branch 10 times, most recently from 1c35c3d to ed7215c Compare December 15, 2023 16:47

LongyuZhang reviewed Jan 3, 2024

View reviewed changes

annaibm force-pushed the update_GitClone_4846 branch 3 times, most recently from 0cbb3c4 to 61cb8cc Compare January 4, 2024 18:33

annaibm requested review from LongyuZhang and karianna January 5, 2024 14:06

karianna reviewed Jan 5, 2024

View reviewed changes

annaibm force-pushed the update_GitClone_4846 branch from 61cb8cc to 747d60f Compare January 5, 2024 21:44

llxia requested changes Jan 8, 2024

View reviewed changes

annaibm force-pushed the update_GitClone_4846 branch 3 times, most recently from 510defa to 3aca90e Compare January 9, 2024 20:18

annaibm force-pushed the update_GitClone_4846 branch from 3aca90e to 2ed66f1 Compare January 19, 2024 18:33

llxia reviewed Jan 19, 2024

View reviewed changes

annaibm force-pushed the update_GitClone_4846 branch 2 times, most recently from 7f8381c to d19fefc Compare January 19, 2024 18:55

llxia reviewed Jan 22, 2024

View reviewed changes

Update git cmd logic in build.xml

08081d5

- Removes prestaged jck material and reclones it, if git command fails resolves: adoptium#4846 Signed-off-by: Anna Babu Palathingal <[email protected]>

annaibm force-pushed the update_GitClone_4846 branch from d19fefc to 08081d5 Compare January 22, 2024 22:09

annaibm closed this Jan 30, 2024

annaibm deleted the update_GitClone_4846 branch March 12, 2024 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update git clone logic in build.xml #4891

Update git clone logic in build.xml #4891

annaibm commented Nov 28, 2023 •

edited

Loading

annaibm commented Nov 28, 2023

LongyuZhang left a comment

annaibm commented Dec 1, 2023 •

edited

Loading

LongyuZhang left a comment

llxia left a comment •

edited

Loading

llxia commented Dec 13, 2023

annaibm commented Dec 18, 2023

LongyuZhang Jan 3, 2024

karianna Jan 5, 2024

annaibm Jan 8, 2024

llxia Jan 8, 2024

llxia commented Jan 8, 2024 •

edited

Loading

llxia Jan 19, 2024

annaibm Jan 22, 2024

llxia Jan 22, 2024

annaibm Jan 22, 2024

llxia Jan 22, 2024

Update git clone logic in build.xml #4891

Update git clone logic in build.xml #4891

Conversation

annaibm commented Nov 28, 2023 • edited Loading

annaibm commented Nov 28, 2023

LongyuZhang left a comment

Choose a reason for hiding this comment

annaibm commented Dec 1, 2023 • edited Loading

LongyuZhang left a comment

Choose a reason for hiding this comment

llxia left a comment • edited Loading

Choose a reason for hiding this comment

llxia commented Dec 13, 2023

annaibm commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llxia commented Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annaibm commented Nov 28, 2023 •

edited

Loading

annaibm commented Dec 1, 2023 •

edited

Loading

llxia left a comment •

edited

Loading

llxia commented Jan 8, 2024 •

edited

Loading