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

feat: support long arguments #12325

Merged
merged 1 commit into from
Jan 13, 2024
Merged

feat: support long arguments #12325

merged 1 commit into from
Jan 13, 2024

Conversation

shuangkun
Copy link
Member

@shuangkun shuangkun commented Dec 6, 2023 •
edited by agilgur5
Loading

offload the envtemplate to configmap when arguments is too long

Fixes #7586
Fixes #12190

Motivation

Modifications

Verification

@saikonen saikonen mentioned this pull request Dec 18, 2023
Closed
@shuangkun shuangkun mentioned this pull request Dec 19, 2023
Closed
5 tasks
@alexec
Copy link
Contributor

alexec commented Jan 8, 2024

Solution seems reasonable. Perhaps this would benefit from some tests? Not sure why this PR has not been reviewed yet.

@sarabala1979 sarabala1979 self-requested a review January 8, 2024 06:38
@sarabala1979
Copy link
Member

I will take a look

@shuangkun
Copy link
Member Author

Perhaps this would benefit from some tests

Solution seems reasonable. Perhaps this would benefit from some tests? Not sure why this PR has not been reviewed yet.

okay, I will add some ut.

@juliev0 juliev0 added the prioritized-review For members of the Sustainability Effort label Jan 9, 2024
@shuangkun shuangkun force-pushed the feat/supportBigArgument branch 5 times, most recently from a51e3c0 to 71db1fe Compare January 10, 2024 06:45
sarabala1979
sarabala1979 reviewed Jan 10, 2024
View reviewed changes
workflow/controller/workflowpod.go Outdated
}

if offloadEnvVarTemplate {
cmName := fmt.Sprintf("%s-configmap", pod.Name)
Copy link
Member

@sarabala1979 sarabala1979 Jan 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

can you add logic to check name is not crossing the maximum limit? if yes, we can short the name.

@shuangkun shuangkun force-pushed the feat/supportBigArgument branch 2 times, most recently from 182d7bf to 2837b32 Compare January 10, 2024 08:08
sarabala1979
sarabala1979 reviewed Jan 10, 2024
View reviewed changes
workflow/util/configmap_name.go Outdated
package util

func GenerateConfigMapName(prefix string) string {
maxPrefixLength := maxK8sResourceNameLength - k8sNamingHashLength
Copy link
Member

Choose a reason for hiding this comment

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

Can you comment on how the name will calculate if it is oversized?
Do we have hash on end?
Eg:
<podname>-confimap > maxPrefixLength

I am thinking,
can we have just only the podname?

@shuangkun @sherwinkoo29 @zjgemi
feat: support long arguments
c430d14 
Co-authored-by: shuangkun <[email protected]>
Co-authored-by: sherwinkoo29 <[email protected]>
Co-authored-by: zjgemi <[email protected]>
Signed-off-by: shuangkun <[email protected]>
sarabala1979
sarabala1979 approved these changes Jan 13, 2024
View reviewed changes
Copy link
Member

@sarabala1979 sarabala1979 left a comment

Choose a reason for hiding this comment

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

LGTM.
@shuangkun thanks for your contribution.

@sarabala1979 sarabala1979 merged commit 85d1c79 into argoproj:main Jan 13, 2024
27 checks passed
@agilgur5 agilgur5 added area/controller Controller issues, panics area/executor labels Jan 14, 2024
@roofurmston roofurmston mentioned this pull request Jan 31, 2024
Closed
@alexflorezr
Copy link

Hi, Is there any plan for releasing this? I was hoping to see it in the new release, but unfortunately it was not there 😢

@Joibel
Copy link
Member

Joibel commented Mar 4, 2024

This is a "feature" so will be in the next minor release (3.6) rather than a patch release.

@alexflorezr
Copy link

alexflorezr commented Mar 5, 2024 •
edited by agilgur5
Loading

Thank you 🙏

@shuangkun shuangkun mentioned this pull request Apr 4, 2024
Closed
6 tasks
@agilgur5 agilgur5 added this to the v3.6.0 milestone May 4, 2024
@jswxstw jswxstw mentioned this pull request May 27, 2024
Open
8 tasks
@fyp711
Copy link
Contributor

fyp711 commented Oct 8, 2024

@shuangkun Thanks for fix it. I have a small question, how to clear this configmap? Is there any way to clean up?

@Joibel
Copy link
Member

Joibel commented Oct 8, 2024 •
edited by agilgur5
Loading

how to clear this configmap? Is there any way to clean up?

It should have an owner reference of the workflow, so get deleted when the workflow is deleted.

@fyp711
Copy link
Contributor

fyp711 commented Oct 8, 2024 •
edited by agilgur5
Loading

I see. Thank you for your answer, which has solved my doubts.

@jswxstw jswxstw mentioned this pull request Oct 12, 2024
Closed
@juliev0 juliev0 mentioned this pull request Oct 14, 2024
Merged
@jswxstw jswxstw mentioned this pull request Oct 16, 2024
Merged
@shuangkun shuangkun mentioned this pull request Nov 1, 2024
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarabala1979 sarabala1979 sarabala1979 approved these changes

Assignees

@sarabala1979 sarabala1979

Labels
area/controller Controller issues, panics area/executor prioritized-review For members of the Sustainability Effort
Projects
None yet
Milestone
v3.6.x patches
8 participants
@shuangkun @alexec @sarabala1979 @alexflorezr @Joibel @fyp711 @agilgur5 @juliev0