Skip to content

Conversation

@Joibel
Copy link
Member

@Joibel Joibel commented Dec 1, 2025

In #14996 the singular version of each of semaphore, mutex would not get moved over to the plural in the presence of both fields.

This is not really correct as if you have both sets in 3.7 you will get the behaviour of the appended list+singular. So this PR makes convert append instead of only replace.

For schedule, if present it would mask schedules. I have kept this behaviour.

Added tests for this.

@Joibel Joibel requested a review from Copilot December 1, 2025 17:12
@Joibel Joibel changed the title fix: always convert singular to plural fix: always convert singular to plural Dec 1, 2025
@Joibel Joibel marked this pull request as draft December 1, 2025 17:13
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the conversion logic for migrating legacy singular fields (semaphore, mutex, schedule) to their plural equivalents (semaphores, mutexes, schedules) in workflow specifications. Previously, the singular fields were only migrated when the plural fields were empty; now they are always appended to ensure both values are preserved when present.

  • Changed conversion strategy from conditional replacement to unconditional append
  • Updated conversion logic for LegacySynchronization.ToCurrent() to always append singular Semaphore and Mutex to their plural forms
  • Updated conversion logic for LegacyCronWorkflowSpec.ToCurrent() to always append singular Schedule to plural Schedules

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@Joibel Joibel marked this pull request as ready for review December 3, 2025 14:27
@Joibel Joibel requested a review from MasonM December 3, 2025 14:27
@coderabbitai
Copy link

coderabbitai bot commented Dec 5, 2025

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@Joibel Joibel merged commit a5b57b1 into argoproj:main Dec 8, 2025
70 of 73 checks passed
Joibel added a commit that referenced this pull request Dec 8, 2025
Signed-off-by: Alan Clucas <[email protected]>
(cherry picked from commit a5b57b1)
@Joibel Joibel added the cherry-pick/3.7 Cherry-pick this to release-3.7 label Dec 8, 2025
@argo-cd-cherry-pick-bot
Copy link

❌ Cherry-pick failed for 3.7. Please check the workflow logs for details.

Joibel added a commit that referenced this pull request Dec 8, 2025
…for 3.7) (#15119)

Signed-off-by: Alan Clucas <[email protected]>
Signed-off-by: Mason Malone <[email protected]>
@Joibel Joibel deleted the convert-fix branch December 8, 2025 13:06
guanguxiansheng pushed a commit to guanguxiansheng/argo-workflows that referenced this pull request Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-pick/3.7 Cherry-pick this to release-3.7

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants