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

Jump to bottom

fix: app names with non-alphanumeric characters in position 63 break syncs (issue #18237) #18256

Merged
merged 3 commits into from
May 22, 2024
Merged

fix: app names with non-alphanumeric characters in position 63 break syncs (issue #18237) #18256

merged 3 commits into from
May 22, 2024

Conversation

RobinsonZ
Copy link
Contributor

@RobinsonZ RobinsonZ commented May 16, 2024 •
edited

Fixes #18237. Fixes #10110.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@RobinsonZ RobinsonZ requested a review from a team as a code owner May 16, 2024 18:47
@RobinsonZ
Ensure truncated app label does not end in a special character
3b88d36 
Signed-off-by: Zack Robinson <robinsoz@arcesium.com>
@jannfis
Copy link
Member

jannfis commented May 16, 2024

This seems trying to fix the same issue as #17560

@RobinsonZ
Copy link
Contributor Author

Seems so. That PR looks stale but I will defer to maintainers as to which version you would like to go with. This is actively biting us, and workarounds are tricky for a variety of reasons, so whatever gets the issue fixed is fine by me.

@RobinsonZ RobinsonZ mentioned this pull request May 16, 2024
Closed
3 tasks
todaywasawesome
todaywasawesome approved these changes May 16, 2024
View reviewed changes
Copy link
Contributor

@todaywasawesome todaywasawesome left a comment

Choose a reason for hiding this comment

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

I think this looks reasonable but I leave it to @jannfis since he looked at the other PR.

I prefer this one a bit because it seems simpler.

jannfis
jannfis reviewed May 17, 2024
View reviewed changes
Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

Thanks. I agree @todaywasawesome, the changes here look a bit simpler and accomplishing the same.

@RobinsonZ I do have some comments, please have a look.

util/argo/resource_tracking.go Outdated Show resolved Hide resolved
util/argo/resource_tracking.go Outdated Show resolved Hide resolved
@RobinsonZ
Move regex to global variable and add out of bounds check
f120800 
Signed-off-by: Zack Robinson <robinsoz@arcesium.com>
@RobinsonZ
Add test for out-of-bounds check
d1590aa 
Signed-off-by: Zack Robinson <robinsoz@arcesium.com>
@RobinsonZ
Copy link
Contributor Author

@jannfis Updated per your feedback. Let me know if any other changes are needed.

@todaywasawesome todaywasawesome merged commit 59ce337 into argoproj:master May 22, 2024
27 checks passed
@RobinsonZ
Copy link
Contributor Author

Can this be cherry-picked into 2.11?

@pasha-codefresh
Copy link
Member

/cherry-pick release-2.11

gcp-cherry-pick-bot bot pushed a commit that referenced this pull request May 28, 2024
@RobinsonZ
fix: app names with non-alphanumeric characters in position 63 break …
3171e84 
…syncs (issue #18237) (#18256)

* Ensure truncated app label does not end in a special character

Signed-off-by: Zack Robinson <robinsoz@arcesium.com>

* Move regex to global variable and add out of bounds check

Signed-off-by: Zack Robinson <robinsoz@arcesium.com>

* Add test for out-of-bounds check

Signed-off-by: Zack Robinson <robinsoz@arcesium.com>

---------

Signed-off-by: Zack Robinson <robinsoz@arcesium.com>
@gcp-cherry-pick-bot gcp-cherry-pick-bot bot mentioned this pull request May 28, 2024
Merged
pasha-codefresh pushed a commit that referenced this pull request May 28, 2024
@gcp-cherry-pick-bot @RobinsonZ
fix: app names with non-alphanumeric characters in position 63 break …
46342a9 
…syncs (issue #18237) (#18256) (#18439)

* Ensure truncated app label does not end in a special character



* Move regex to global variable and add out of bounds check



* Add test for out-of-bounds check



---------

Signed-off-by: Zack Robinson <robinsoz@arcesium.com>
Co-authored-by: Zack Robinson <zkislakrobinson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jannfis jannfis jannfis left review comments

@todaywasawesome todaywasawesome todaywasawesome approved these changes

@pasha-codefresh pasha-codefresh pasha-codefresh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@RobinsonZ @jannfis @pasha-codefresh @todaywasawesome