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(executor): Fix resource patch when not providing flags. Fixes #5310 #5311

Merged
merged 1 commit into from
Mar 11, 2021
Merged

fix(executor): Fix resource patch when not providing flags. Fixes #5310 #5311

merged 1 commit into from
Mar 11, 2021

Conversation

terrytangyuan
Copy link
Member

Signed-off-by: terrytangyuan terrytangyuan@gmail.com

Checklist:

@terrytangyuan terrytangyuan marked this pull request as ready for review March 5, 2021 15:59
@terrytangyuan
Copy link
Member Author

Seems like a bug introduced in #4908 and a regression since v3.0.0-rc1.

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

@book987 can you please take a look?

workflow/executor/resource_test.go Outdated Show resolved Hide resolved
@terrytangyuan terrytangyuan requested a review from alexec March 9, 2021 15:36
Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>
@terrytangyuan
Copy link
Member Author

terrytangyuan commented Mar 10, 2021

Rebased and finally passed the CI. @alexec Thanks for making the CI more robust! I think this is ready to go unless I missed anything else.

@alexec alexec merged commit e0f71f3 into argoproj:master Mar 11, 2021
@terrytangyuan terrytangyuan deleted the fix-resource-patch branch March 11, 2021 01:45
@simster7 simster7 mentioned this pull request Mar 15, 2021
27 tasks
@simster7 simster7 mentioned this pull request Mar 29, 2021
77 tasks
@simster7 simster7 mentioned this pull request Apr 19, 2021
50 tasks
@wreed4
Copy link
Contributor

wreed4 commented May 19, 2021

@alexec it seems like this fix is still not available in 3.0.1, 3.0.2, or 3.0.3. I can see the cherry-picks above, but I can't see it in the changelogs, and most importantly, we have verified the bug still exists in all three of those versions. believe we tested 3.0.4 as well but not positive. Can you give any insight into when this will be available?

@wreed4
Copy link
Contributor

wreed4 commented May 19, 2021

upon further looking actually.... looks like this particular code is there, but the syntax for patch must have changed from 2.X?? Are there docs on that I can't seem to find them. I'm now getting this error: Error (exit code 1): error: You must provide one or more resources by argument or filename. with this patch

  - name: patch
    resource:
      action: patch
      manifest: |
        apiVersion: v1
        kind: ConfigMap
        metadata:
          name: patch-test-{{workflow.name}}
          annotations:
            patched: "true"

the same code works in 2.x

@wreed4
Copy link
Contributor

wreed4 commented May 19, 2021

It's this line here https://github.com/argoproj/argo-workflows/blob/master/workflow/executor/resource.go#L107
It prevents passing the file so kubectl doesn't know which object to pass. I'd definitely consider this a regression. Docs seems like they are not updated either to reflect an intentional change. https://github.com/argoproj/argo-workflows/tree/v3.0.4/examples#kubernetes-resources

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants