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.

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

feat(RELEASE-1252): make run-file-updates task idempotent #727

Open
wants to merge 1 commit into
base: development
Choose a base branch
from
+187 −4
Open

feat(RELEASE-1252): make run-file-updates task idempotent #727

Show file tree
Hide file tree
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
3 changes: 3 additions & 0 deletions internal/tasks/process-file-updates-task/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ replacements to a yaml file that already exists. It will attempt to create a Mer
| tempDir | temp dir for cloning and updates | Yes | /tmp/$(context.taskRun.uid)/file-updates |
| internalRequestPipelineRunName | name of the PipelineRun that called this task | No | - |

## Changes in 0.1.0
* make run-file-updates task idempotent

## Changes in 0.0.2
* add new `internalRequestPipelineRunName` parameter and result
- Tekton only supports passing task results as pipeline results,
Expand Down
62 changes: 58 additions & 4 deletions internal/tasks/process-file-updates-task/process-file-updates-task.yaml
View file
Open in desktop
johnbieren marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ kind: Task
metadata:
name: process-file-updates-task
labels:
app.kubernetes.io/version: "0.0.2"
app.kubernetes.io/version: "0.1.0"
annotations:
tekton.dev/pipelines.minVersion: "0.12.1"
tekton.dev/tags: release
Expand Down Expand Up @@ -160,6 +160,7 @@ spec:
exit 1
fi

keyNotFound=false
for (( REPLACEMENT_INDEX=0; REPLACEMENT_INDEX < REPLACEMENTS_LENGTH; REPLACEMENT_INDEX++ )); do
echo "REPLACEMENT: #${REPLACEMENT_INDEX}"
key="$(jq -cr ".[${PATH_INDEX}].replacements[${REPLACEMENT_INDEX}].key" "${updatePathsTmpfile}")"
Expand All @@ -173,6 +174,7 @@ spec:
foundAt=$(head -n 1 "${TEMP}/found.txt")
if (( foundAt == 0 )); then
echo "NOT FOUND"
keyNotFound=true
continue
fi
echo "FOUND"
Expand Down Expand Up @@ -236,14 +238,66 @@ spec:
error="\"no replacements were performed\"" \
yq -o json --null-input '.str = strenv(error), .error = strenv(error)' \
| tee "$(results.fileUpdatesInfo.path)"
echo -n "Failed" |tee "$(results.fileUpdatesState.path)"
if [[ "$keyNotFound" == true ]]; then
echo -n "Failed" |tee "$(results.fileUpdatesState.path)"
else
echo -n "Success" |tee "$(results.fileUpdatesState.path)"
fi
# it should exit 0 otherwise the task does not set the results
# this way the InternalRequest can see what was wrong
exit 0
fi

echo -e "\n*** START LOCAL CHANGES ***\n"
git diff
git diff | tee "${TEMP}"/tempMRFile.diff

if [[ -s "${TEMP}"/tempMRFile.diff ]]; then
# replacements exist
# It's to deal with the lines like "@@ -N1,N2 +N3,N4 @@",
# but it's possible to meet a line like "@@ -4,7 +4,7 @@ $schema: /app/app-settings.yml".
awk '/^@@/ {match($0, /@@ ([^@]+) @@/, arr); print arr[1]}' "${TEMP}"/tempMRFile.diff |\
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do? Can you add a comment explaining what it will print?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's to deal with a line like "@@ -4,7 +4,7 @@ $schema: /app-interface/app-interface-settings-1.yml". Usually it should be "@@ -4,7 +4,7 @@" in a line and " $schema: /app-interface/app-interface-settings-1.yml" in a seperated line.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't get it 😂 but can you make that comment in the code? This will be lost once this is merged

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK. Added a comment.

tee "${TEMP}"/lineFile
fi

openMRList=$(glab mr list -R "${UPSTREAM_REPO}" --search "Konflux release" |grep "^!"| cut -f1 | tr -d '!')
for mrNum in ${openMRList} ; do
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me this whole logic is only for the use case that there is replacement to be done, but maybe one of the open MRs already contains it. That's definitely a valid use case.

But what if the change has already been merged? Then we will probably fail on line 235, saying there is nothing to change, right? But if you run the same release repeatedly, that use case shouldn't fail.

Now the question is how to handle this - we will need to separate these two cases:

  • there is substitution to be done because the files already contain the new strings -> pass
  • there is substitution to be done for some other reason, e.g. we didn't find the lines to be updated -> fail

Copy link
Collaborator Author

@jinqi7 jinqi7 Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But what if the change has already been merged? Then we will probably fail on line 235, saying there is nothing to change, right? But if you run the same release repeatedly, that use case shouldn't fail.

How to differentiate the MR is merged and the file doesn't need to be changed at all? Do we still need to check the merged PRs?

Copy link
Contributor

Choose a reason for hiding this comment

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

We might need to discuss this with the whole team to decide how to handle this. Originally when this was implemented, the idea was that if there is nothing to be updated, then there's definitely something wrong. But now that we want to take a release pipeline rerun into account, we shouldn't automatically fail on this. We will need to be more granular. If we can find the file to update and then we search for the line to update and find it, but the line already contains the target string, then that's still ok. We only fail if we cannot find the file to update or cannot match the line to update.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need to check the merged PRs?

I guess that would be one way to handle it. But probably not necessary. It would be difficult to decide how far into the history we want to go.

Copy link
Collaborator

@johnbieren johnbieren Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, if the "goal string" aka the string you are trying to insert already exists in the proper file, exit 0. No need to check merged MRs. If an MR is open with the exact change already, say that (idempotent) and exit 0 (ideally printing the existing MR url). If the file to be changed doesn't exist, exit 1. I think this is in agreement with what Martin has been commenting

Copy link
Collaborator

Choose a reason for hiding this comment

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

But yes, the case where the change is already merged seems already addressed. It just has to be updated to no longer fail (the line 235 piece Martin mentioned)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If only the return value is changed from "Failed" to "Success" in the block of "line 235", the test case of "test-process-file-updates-replacements-error.yaml" fails. I added a variable of "keyNotFound" to check if the key is correct. And if the file is not valid, it's handled in line 157. Is that ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am reading through what the script does and I am not sure now. Until now, if a key is not found, that replacement will be skipped, but at the end we check that at least one replacement happened, otherwise we fail.

So yes, I think what you say sounds correct - we should fail if we cannot find the target file or the target key. But if those are found and there is no diff once the replacement is applied, it's still ok.

But I noticed there is also this seed option. It seems that one should be ok - if you run it again, it will just override the file with the seed string again, so there will be no diff. Previously this would fail on "no diff", but as we said, we will no longer fail on "no diff".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Until now, if a key is not found, that replacement will be skipped, but at the end we check that at least one replacement happened, otherwise we fail.

I didn't change the logic of seed/replacement before, just add/set the "keyNotFound" variable. If there is no replacement and all the keys can be found, it return true because the MR may be merged. If some key can't be found, it fails.

foundMR=false
glab mr diff "${mrNum}" --repo "${UPSTREAM_REPO}" > "${TEMP}"/oneMR.diff

if [[ ! -s "${TEMP}"/lineFile ]]; then
# only seed files
grep '^[+][^+]' "${TEMP}"/oneMR.diff | sed -E 's/^[+]//' | tee "${TEMP}/mrFile"

# compare if the contents and the updated file names are the same
if diff -q "${TEMP}"/mrFile "${targetFile}" > /dev/null; then
grep "^diff --git" "${TEMP}"/oneMR.diff | tee "${TEMP}"/tmpFile
while read -r line ; do
changedFileName=$(echo "$line" | awk '{sub(/^b\//, "", $NF); print $NF}')
if [[ "${changedFileName}" != "${targetFile}" ]]; then
continue
fi
foundMR=true
break
done < "${TEMP}"/tmpFile
fi
else
# replacements exist
grep '^[-+@]' "${TEMP}"/oneMR.diff | sed -E 's/^[@]//; s/@@.*//' | tee "${TEMP}/mrFile"
grep '^[-+@]' "${TEMP}"/tempMRFile.diff | sed -E 's/^[@]//; s/@@.*//' | tee "${TEMP}/tmpFile"
if diff -q "${TEMP}"/mrFile "${TEMP}"/tmpFile > /dev/null; then
foundMR=true
fi
fi
# if all the lines are matched, the MR is the same one
# it should exit 0 if the same MR exists
if [[ "$foundMR" == true ]]; then
theflockers marked this conversation as resolved.
Show resolved Hide resolved
echo "there is an existing MR with the same updates in the repo"
echo "{\"merge_request\":\"${UPSTREAM_REPO}/-/merge_requests/${mrNum}\"}" \
| tee -a "$(results.fileUpdatesInfo.path)"
echo -n "Success" | tee "$(results.fileUpdatesState.path)"
exit 0
fi
done
echo -e "\n*** END LOCAL CHANGES ***\n"

WORKING_BRANCH=$(uuidgen |awk '{print substr($1, 1, 8)}')
Expand All @@ -253,7 +307,7 @@ spec:
GITLAB_MR_MSG="[Konflux release] $(params.application): fileUpdates changes ${WORKING_BRANCH}"
gitlab_create_mr --head "$WORKING_BRANCH" --target-branch "$REVISION" --title "${GITLAB_MR_MSG}" \
--description "${GITLAB_MR_MSG}" --upstream-repo "${UPSTREAM_REPO}" | jq '. | tostring' \
|tee -a "$(results.fileUpdatesInfo.path)"
| tee -a "$(results.fileUpdatesInfo.path)"

echo -n "Success" |tee "$(results.fileUpdatesState.path)"

Expand Down
22 changes: 22 additions & 0 deletions internal/tasks/process-file-updates-task/tests/mocks.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,33 @@ function git() {
if [[ "$*" == "config"* ]]; then
/usr/bin/git "$@"
fi
if [[ "$*" == "diff"* ]]; then
/usr/bin/git "$@"
fi
}

function glab() {
if [[ "$*" == *"mr create"* ]]; then
gitRepo=$(echo "$*" | cut -f5 -d/ | cut -f1 -d.)
echo "/merge_request/1"
elif [[ "$*" == *"mr list"* ]]; then
echo '!1'
elif [[ "$*" == *"mr diff"* ]]; then
gitRepo=$(echo "$*" | cut -f5 -d/ | cut -f1 -d.)
if [[ "${gitRepo}" == "replace-idempotent" ]]; then
echo "diff --git a/addons/my-addon2.yaml b/addons/my-addon2.yaml
--- a/addons/my-addon2.yaml
+++ b/addons/my-addon2.yaml
@@ -1,2 +1,2 @@
-indexImage:
+indexImage: Jack
"
else
echo "diff --git a/test/one-update.yaml b/test/one-update.yaml
+++ b/test/one-update.yaml
@@ -1,2 +1,2 @@
+indexImage: Jack
"
fi
fi
}
104 changes: 104 additions & 0 deletions ...ks/process-file-updates-task/tests/test-process-file-updates-replacements-idempotent.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
---
apiVersion: tekton.dev/v1
kind: Pipeline
metadata:
name: test-process-file-updates-replacements-idempotent
spec:
description: |
Run the process-file-updates task with replacements. No commit
is created if there is a mocked MR with the same content.
workspaces:
- name: tests-workspace
tasks:
- name: setup
workspaces:
- name: pipeline
workspace: tests-workspace
taskSpec:
workspaces:
- name: pipeline
steps:
- name: setup-values
image: quay.io/konflux-ci/release-service-utils:e633d51cd41d73e4b3310face21bb980af7a662f
script: |
#!/usr/bin/env bash
set -eux
mkdir -p "$(workspaces.pipeline.path)/$(context.pipelineRun.uid)/file-updates"
cd "$(workspaces.pipeline.path)/$(context.pipelineRun.uid)/file-updates"
mkdir replace-idempotent
cd replace-idempotent
git config --global init.defaultBranch main
git init .
git config --global user.email "[email protected]"
git config --global user.name "tester"
mkdir addons
cat > "addons/my-addon2.yaml" << EOF
indexImage:
name: test
EOF
git add addons/my-addon2.yaml
git commit -m "prior commit"
- name: run-task
taskRef:
name: process-file-updates-task
params:
- name: upstream_repo
value: "https://some.gitlab/test/replace-idempotent"
- name: repo
value: "https://some.gitlab/test/replace-idempotent"
- name: ref
value: "main"
- name: paths
value: >-
[{"path":"addons/my-addon2.yaml","replacements":[{"key":".indexImage",
"replacement":"|indexImage.*|indexImage: Jack|"}]}]
- name: application
value: "scott"
- name: file_updates_secret
value: "file-updates-secret"
- name: tempDir
value: "$(workspaces.pipeline.path)/$(context.pipelineRun.uid)/file-updates"
- name: internalRequestPipelineRunName
value: $(context.pipelineRun.name)
workspaces:
- name: pipeline
workspace: tests-workspace
runAfter:
- setup
- name: check-result
runAfter:
- run-task
params:
- name: fileUpdatesInfo
value: $(tasks.run-task.results.fileUpdatesInfo)
- name: fileUpdatesState
value: $(tasks.run-task.results.fileUpdatesState)
- name: tempDir
value: "$(workspaces.pipeline.path)/$(context.pipelineRun.uid)/file-updates"
taskSpec:
params:
- name: fileUpdatesInfo
type: string
- name: fileUpdatesState
type: string
- name: tempDir
type: string
steps:
- name: check-result
image: quay.io/konflux-ci/release-service-utils:e633d51cd41d73e4b3310face21bb980af7a662f
script: |
#!/usr/bin/env bash
set -eux
cd "$(params.tempDir)/replace-idempotent"
commits=$(git log --oneline | wc -l)

echo "Test no more commit created except the one in setup step"
test "${commits}" == "1"
rm -rf "$(params.tempDir)"

workspaces:
- name: pipeline
workspace: tests-workspace
Loading