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

jinqi7 · 2024-12-05T04:10:46Z

The PR is to make the task idempotent by ensuring that a new MR isn't
created unnecessarily when one already exists with the same content
in the seed and replacements of paths parameter.
And it added some logic to check the keys in the paths parameter. If the
keys exist but their content does not require any replacement, return
success.

openshift-ci · 2024-12-05T04:10:50Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

jinqi7 · 2024-12-06T04:52:54Z

/retest

jinqi7 · 2024-12-10T02:19:38Z

/retest

jinqi7 · 2024-12-10T03:35:40Z

/retest

jinqi7 · 2024-12-10T04:53:15Z

/retest

theflockers · 2024-12-10T08:48:28Z

internal/tasks/process-file-updates-task/process-file-updates-task.yaml

+            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


Could you add another echo here saying that an existent MR for the same change was found, so the pipeline user would know why his new MR was not created - or think this is a new MR.

Yes. Added.

mmalina · 2024-12-10T12:18:51Z

internal/tasks/process-file-updates-task/README.md

Can you provide more details in the commit message and PR description? At least some basic description of how it's being made idempotent.

mmalina · 2024-12-10T12:27:53Z

internal/tasks/process-file-updates-task/process-file-updates-task.yaml

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


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

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?

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.

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.

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

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)

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?

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".

mmalina · 2024-12-10T12:28:23Z

internal/tasks/process-file-updates-task/process-file-updates-task.yaml

+
+        if [[ -s "${TEMP}"/tempMRFile.diff ]]; then
+            # replacements exist
+            awk '/^@@/ {match($0, /@@ ([^@]+) @@/, arr); print arr[1]}' "${TEMP}"/tempMRFile.diff |\


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

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.

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

OK. Added a comment.

johnbieren · 2024-12-12T16:38:11Z

internal/tasks/process-file-updates-task/process-file-updates-task.yaml

+
+            if [[ ! -s "${TEMP}"/lineFile ]]; then
+                # only seed files  
+                foundMR=false


Is this needed since it is set on line 264?

Removed. Thanks

Signed-off-by: Jing Qi <jinqi@redhat.com> The PR is to make the task idempotent by ensuring that a new MR isn't created unnecessarily when one already exists with the same content in the seed and replacements of paths parameter. And it added some logic to check the keys in the paths parameter. If the keys exist but their content does not require any replacement, return success.

openshift-ci bot added the do-not-merge/work-in-progress label Dec 5, 2024

jinqi7 force-pushed the RELEASE-1252 branch 14 times, most recently from 5cc7967 to c315bf0 Compare December 6, 2024 04:30

jinqi7 marked this pull request as ready for review December 6, 2024 04:52

jinqi7 requested a review from a team as a code owner December 6, 2024 04:52

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 6, 2024

jinqi7 force-pushed the RELEASE-1252 branch from c315bf0 to 90d3e4f Compare December 6, 2024 04:53

jinqi7 marked this pull request as draft December 6, 2024 06:43

openshift-ci bot added the do-not-merge/work-in-progress label Dec 6, 2024

jinqi7 force-pushed the RELEASE-1252 branch 7 times, most recently from 633faea to 2aa1a0e Compare December 6, 2024 11:11

jinqi7 force-pushed the RELEASE-1252 branch 6 times, most recently from c92d6fd to 3336302 Compare December 10, 2024 00:47

jinqi7 marked this pull request as ready for review December 10, 2024 00:47

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 10, 2024

theflockers reviewed Dec 10, 2024

View reviewed changes

mmalina reviewed Dec 10, 2024

View reviewed changes

jinqi7 force-pushed the RELEASE-1252 branch 5 times, most recently from 2327d8a to 5904751 Compare December 11, 2024 12:45

johnbieren reviewed Dec 12, 2024

View reviewed changes

jinqi7 force-pushed the RELEASE-1252 branch 2 times, most recently from 7feb933 to 1cb83b1 Compare December 13, 2024 04:10

jinqi7 marked this pull request as draft December 13, 2024 04:51

openshift-ci bot added the do-not-merge/work-in-progress label Dec 13, 2024

jinqi7 force-pushed the RELEASE-1252 branch from 1cb83b1 to 8ba7160 Compare December 13, 2024 04:53

jinqi7 marked this pull request as ready for review December 13, 2024 04:58

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 13, 2024

jinqi7 force-pushed the RELEASE-1252 branch from 8ba7160 to 4df7129 Compare December 13, 2024 05:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jinqi7 commented Dec 5, 2024 •

edited

Loading

openshift-ci bot commented Dec 5, 2024

jinqi7 commented Dec 6, 2024

jinqi7 commented Dec 10, 2024

jinqi7 commented Dec 10, 2024

jinqi7 commented Dec 10, 2024

theflockers Dec 10, 2024 •

edited

Loading

jinqi7 Dec 11, 2024

mmalina Dec 10, 2024

jinqi7 Dec 13, 2024

mmalina Dec 10, 2024

jinqi7 Dec 10, 2024 •

edited

Loading

mmalina Dec 10, 2024

mmalina Dec 10, 2024

johnbieren Dec 10, 2024 •

edited

Loading

johnbieren Dec 10, 2024

jinqi7 Dec 11, 2024

mmalina Dec 13, 2024

mmalina Dec 10, 2024

jinqi7 Dec 10, 2024

johnbieren Dec 10, 2024

jinqi7 Dec 11, 2024

johnbieren Dec 12, 2024

jinqi7 Dec 13, 2024

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

Are you sure you want to change the base?

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

Conversation

jinqi7 commented Dec 5, 2024 • edited Loading

openshift-ci bot commented Dec 5, 2024

jinqi7 commented Dec 6, 2024

jinqi7 commented Dec 10, 2024

jinqi7 commented Dec 10, 2024

jinqi7 commented Dec 10, 2024

theflockers Dec 10, 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

jinqi7 Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnbieren Dec 10, 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

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

jinqi7 commented Dec 5, 2024 •

edited

Loading

theflockers Dec 10, 2024 •

edited

Loading

jinqi7 Dec 10, 2024 •

edited

Loading

johnbieren Dec 10, 2024 •

edited

Loading