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.

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

fix(controller): re-allow changing executor args #12609

Merged
merged 2 commits into from
Feb 17, 2024

Conversation

agilgur5
Copy link

@agilgur5 agilgur5 commented Feb 2, 2024

Fixes #12315

Motivation

  • this was unintentionally removed in feat!: Remove deprecated config flags. Fixes #7971 #8009 (see my review comment), which was meant to remove deprecated config flags and accidentally removed this as well
    • it was most certainly on accident as the PR description & issue description don't mention this, and the docs were left as-is for executor args
    • unfortunately this means that 3.3 was missing this entirely and current versions of 3.4 and 3.5 were too
      • should be able to backport this to 3.4.x at least and get into a 3.5.x patch release

See my root cause analysis in #12315 (comment)

Modifications

  • add back the functionality to configure args for the Executor

  • also add a (regression) test for customizing the Executor container

    • the args as well as resources.limits, since those were previously untested in this file
    • named the same as TestMainContainerCustomization
  • also fixes a misnamed test in the same file

Verification

  • added a new test for some previously untested executor container customizations

- this was unintentionally removed in b7a525b, which was meant to remove deprecated config flags and accidentally removed this as well
  - it was most certainly on accident as the PR description & issue description don't mention this, and the docs were left as-is for executor `args`
  - unfortunately this means that 3.3 was missing this entirely and current versions of 3.4 and 3.5 were too
    - should be able to backport this to 3.4.x at least and get into a 3.5.x patch release

- add back the functionality to configure `args` for the Executor
- also add a (regression) test for customizing the Executor container
  - the `args` as well as `resources.limits`, since those were previously untested in this file
  - named the same as `TestMainContainerCustomization`

Signed-off-by: Anton Gilgur <[email protected]>
@agilgur5 agilgur5 requested a review from sarabala1979 February 2, 2024 22:43
@agilgur5 agilgur5 added the area/controller Controller issues, panics label Feb 2, 2024
@agilgur5
Copy link
Author

agilgur5 commented Feb 2, 2024

Unrelated E2E test failure seems to be due to #12413, c.f. #12596

@isubasinghe
Copy link
Member

Hmm this test failure is kinda annoying, do you have any opinions on the review process when there is a flaky test @agilgur5?
I think perhaps urgent PRs can be expedited but others have to wait until the tests pass?

@agilgur5
Copy link
Author

agilgur5 commented Feb 3, 2024

We can't skip tests to merge, they're required CI checks after all. Only repo admins might be able to skip them (which I assume is only Leads, but idk).
In the case of admins, I think it's fine if we have agreement that it's a test flake.

But realistically we really need to fix or skip a flake that's hit this frequently -- i.e. the flake is one of the highest priority since it blocks merging, causes confusion, etc.

@isubasinghe
Copy link
Member

We can't skip tests to merge, they're required CI checks after all. Only repo admins might be able to skip them (which I assume is only Leads, but idk). In the case of admins, I think it's fine if we have agreement that it's a test flake.

Ah okay I thought we'd be getting those rights after the membership changes, perhaps not something we need to think about in that case.

But realistically we really need to fix or skip a flake that's hit this frequently -- i.e. the flake is one of the highest priority since it blocks merging, causes confusion, etc.

Yeah I agree on this, actually I have some opinions on e2e testing, I will open an issue about that.

@agilgur5
Copy link
Author

agilgur5 commented Feb 4, 2024

Ah okay I thought we'd be getting those rights after the membership changes, perhaps not something we need to think about in that case.

I believe we'd have at least the "Write" role, not sure about "Maintain" or "Admin" roles

@agilgur5
Copy link
Author

agilgur5 commented Feb 5, 2024

Merged main to fix E2Es as #12596 is now merged

@juliev0 juliev0 merged commit 23f8c35 into argoproj:main Feb 17, 2024
27 checks passed
@agilgur5 agilgur5 deleted the fix-controller-override-exec-args branch February 17, 2024 08:07
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request Feb 20, 2024
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request Feb 27, 2024
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request Feb 28, 2024
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Isitha Subasinghe <[email protected]>
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request Mar 12, 2024
@agilgur5 agilgur5 added this to the v3.5.x patches milestone Apr 3, 2024
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request May 6, 2024
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller Controller issues, panics area/executor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't configure Executor args. Is it intended?
3 participants