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

WorkflowEventBinding passes raw go structs instead of json marshalled payload params #4572

Closed
nojvek opened this issue Nov 20, 2020 · 7 comments · Fixed by #4594
Closed

WorkflowEventBinding passes raw go structs instead of json marshalled payload params #4572

nojvek opened this issue Nov 20, 2020 · 7 comments · Fixed by #4594
Assignees
Labels
Milestone

Comments

@nojvek
Copy link
Contributor

nojvek commented Nov 20, 2020

Summary

What happened/what you expected to happen?
Creating a simple WorkflowEventBinding for github that triggers workflows based on certain labels in PR.

Doing payload.pull_request.labels gives me a full go struct rather than a json object. Go structs aren't very useful since the lingua franca of the web is json. I can't easily pipe this into a python container and do something with labels.

Diagnostics

apiVersion: argoproj.io/v1alpha1
kind: WorkflowTemplate
metadata:
  annotations:
    owner: core
  labels: {}
  name: deploy-pr
  namespace: github
spec:
  arguments:
    artifacts: []
    parameters:
    - name: action
      value: ''
    - name: pr_author
      value: ''
    - name: pr_branch
      value: ''
    - name: pr_commit
      value: ''
    - name: pr_labels
      value: ''
    - name: pr_num
      value: ''
  entrypoint: deploy-pr
  templates:
  - container:
      args:
      - '{{workflow.parameters}}'
      command:
      - echo
      image: ubuntu:latest
      imagePullPolicy: IfNotPresent
    name: print
  - dag:
      tasks:
      - arguments:
          artifacts: []
          parameters: []
        name: trigger-workflow
        template: print
    name: deploy-pr
  volumes: []
  workflowMetadata:
    annotations:
      owner: core
    labels: {}
---
apiVersion: argoproj.io/v1alpha1
kind: WorkflowEventBinding
metadata:
  annotations:
    owner: core
  labels: {}
  name: deploy-pr
  namespace: github
spec:
  event:
    selector: metadata["x-github-event"] == ["pull_request"] && any(payload.pull_request.labels,
      {.name == "stage-and-deploy"})
  submit:
    arguments:
      parameters:
      - name: action
        valueFrom:
          event: payload.action
      - name: pr_author
        valueFrom:
          event: payload.pull_request.user.login
      - name: pr_branch
        valueFrom:
          event: payload.pull_request.head.ref
      - name: pr_commit
        valueFrom:
          event: payload.pull_request.head.sha
      - name: pr_labels
        valueFrom:
          event: payload.pull_request.labels
      - name: pr_num
        valueFrom:
          event: payload.number
    workflowTemplateRef:
      name: deploy-pr

What version of Argo Workflows are you running? v2.11.5

The output is

[
  {
    "name": "action",
    "value": "labeled"
  },
  {
    "name": "pr_author",
    "value": "nojvek"
  },
  {
    "name": "pr_branch",
    "value": "nojvek-test"
  },
  {
    "name": "pr_commit",
    "value": "f2116e4139e9ef8827f65b76293890e594f0fdcb"
  },
  {
    "name": "pr_labels",
    "value": "[map[color:0052cc default:false description:Only deploy configmaps id:2.442548891e+09 name:configmaps-only node_id:MDU6TGFiZWwyNDQyNTQ4ODkx url:https://api.github.com/repos/mixpanel/analytics/labels/configmaps-only] map[color:1d76db default:false description:Don't wait for user QA, if staging workflow succeeds, deploy to prod id:2.514491821e+09 name:skip-qa-confirm node_id:MDU6TGFiZWwyNTE0NDkxODIx url:https://api.github.com/repos/mixpanel/analytics/labels/skip-qa-confirm] map[color:5319e7 default:false description:Stage to staging-1, then deploy to production id:2.442682022e+09 name:stage-and-deploy node_id:MDU6TGFiZWwyNDQyNjgyMDIy url:https://api.github.com/repos/mixpanel/analytics/labels/stage-and-deploy] map[color:0052cc default:false description: id:2.292237044e+09 name:webapp/nginx node_id:MDU6TGFiZWwyMjkyMjM3MDQ0 url:https://api.github.com/repos/mixpanel/analytics/labels/webapp/nginx]]"
  },
  {
    "name": "pr_num",
    "value": "33195"
  }
]

The core issue is I'd expect the payload.pull_request.labels to be json marshalled, rather than go debug output e.g [map[color:0052cc


Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritise the issues with the most 👍.

@nojvek
Copy link
Contributor Author

nojvek commented Nov 20, 2020

I've done some debugging. It seems the issue is at this line.

https://github.com/argoproj/argo/blob/176d890c1cac25856f67fbed4cc39a396aa87a93/server/event/dispatch/operation.go#L116

Rather than having an AnyStringPtr, we should probably jsonMarshall if the value is non string

@alexec
Copy link
Contributor

alexec commented Nov 20, 2020

@nojvek can you please provide an example of the payload?

@alexec
Copy link
Contributor

alexec commented Nov 20, 2020

I've created an engineering build:

argoproj/argocli:whmap

This is based on master.

@nojvek
Copy link
Contributor Author

nojvek commented Nov 25, 2020

@alexec thanks for looking into this. The payload is a github payload for pull_request. Looks like this https://developer.github.com/webhooks/event-payloads/#pull_request

{
  "action": "opened",
  "number": 2,
  "pull_request": {
    "url": "https://api.github.com/repos/Codertocat/Hello-World/pulls/2",
    "id": 279147437,
    "node_id": "MDExOlB1bGxSZXF1ZXN0Mjc5MTQ3NDM3",
    "html_url": "https://github.com/Codertocat/Hello-World/pull/2",
    "diff_url": "https://github.com/Codertocat/Hello-World/pull/2.diff",
    "patch_url": "https://github.com/Codertocat/Hello-World/pull/2.patch",
    "issue_url": "https://api.github.com/repos/Codertocat/Hello-World/issues/2",
    "number": 2,
    "state": "open",
    "locked": false,
    "title": "Update the README with new information.",
    "user": {
      "login": "Codertocat",
      "id": 21031067,
      "node_id": "MDQ6VXNlcjIxMDMxMDY3",
      "avatar_url": "https://avatars1.githubusercontent.com/u/21031067?v=4",
      "gravatar_id": "",
      "url": "https://api.github.com/users/Codertocat",
      "html_url": "https://github.com/Codertocat",
      "followers_url": "https://api.github.com/users/Codertocat/followers",
      "following_url": "https://api.github.com/users/Codertocat/following{/other_user}",
      "gists_url": "https://api.github.com/users/Codertocat/gists{/gist_id}",
      "starred_url": "https://api.github.com/users/Codertocat/starred{/owner}{/repo}",
      "subscriptions_url": "https://api.github.com/users/Codertocat/subscriptions",
      "organizations_url": "https://api.github.com/users/Codertocat/orgs",
      "repos_url": "https://api.github.com/users/Codertocat/repos",
      "events_url": "https://api.github.com/users/Codertocat/events{/privacy}",
      "received_events_url": "https://api.github.com/users/Codertocat/received_events",
      "type": "User",
      "site_admin": false
    },
    "body": "This is a pretty simple change that we need to pull into master.",
    "created_at": "2019-05-15T15:20:33Z",
    "updated_at": "2019-05-15T15:20:33Z",
    "closed_at": null,
    "merged_at": null,
    "merge_commit_sha": null,
    "assignee": null,
    "assignees": [

    ],
    "requested_reviewers": [

    ],
    "requested_teams": [

    ],
    "labels": [
     {
          "id": 1362937026,
          "node_id": "MDU6TGFiZWwxMzYyOTM3MDI2",
          "url": "https://api.github.com/repos/Codertocat/Hello-World/labels/:bug:%20Bugfix",
          "name": ":bug: Bugfix",
         "color": "cceeaa",
         "default": false,
         "description": "This bug needs fixing"
       }
    ],
...
}

My intent was to capture payload.pull_request.labels and have it sent to the workflow as a json param.

     - name: pr_labels
        valueFrom:
          event: payload.pull_request.labels

Actual: (go formatted struct)

    [map[color:cceeaa default:false description:This bug needs fixing id:1.342548891e+09 name::bug: Bugfix MDU6TGFiZWwxMzYyOTM3MDI2 url:https://api.github.com/repos/Codertocat/Hello-World/labels/:bug:%20Bugfix]]"

Expected: (json string)

[
     {
          "id": 1362937026,
          "node_id": "MDU6TGFiZWwxMzYyOTM3MDI2",
          "url": "https://api.github.com/repos/Codertocat/Hello-World/labels/:bug:%20Bugfix",
          "name": ":bug: Bugfix",
         "color": "cceeaa",
         "default": false,
         "description": "This bug needs fixing"
       }
    ]

@alexec
Copy link
Contributor

alexec commented Nov 26, 2020

OK. Can you please test the engineer build mentioned in my comment has addressed this please?

@nojvek
Copy link
Contributor Author

nojvek commented Nov 27, 2020

Hi @alexec, I don't manage our argo install so not sure how to test this. If it's working in your tests, that is good enough. We will update to next release of argo when it's out.

alexec added a commit that referenced this issue Dec 2, 2020
alexcapras pushed a commit to alexcapras/argo that referenced this issue Dec 2, 2020
Signed-off-by: [email protected] <[email protected]>

feat(ui): Add Template/Cron workflow filter to workflow page. Closes argoproj#4532 (argoproj#4543)

Signed-off-by: Tianchu Zhao <[email protected]>

feat(executor): Auto create s3 bucket if not present.

Signed-off-by: Alex Capras <[email protected]>

Apply codegen

Signed-off-by: Alex Capras <[email protected]>

Add argo-e2e label to test wf

Signed-off-by: Alex Capras <[email protected]>

chore: Updated stress test YAML (argoproj#4569)

Signed-off-by: Alex Collins <[email protected]>

docs: Updated kubectl apply command in manifests README (argoproj#4577)

Signed-off-by: Stefan Gloutnikov <[email protected]>

feat(controller): Make MAX_OPERATION_TIME configurable. Close argoproj#4239 (argoproj#4562)

Signed-off-by: Alex Collins <[email protected]>

docs: Fix a typo in example (argoproj#4590)

Signed-off-by: Takayoshi Nishida <[email protected]>

feat(controller): Retry transient offload errors. Resolves argoproj#4464 (argoproj#4482)

Signed-off-by: Alex Collins <[email protected]>

fix(server): use the correct name when downloading artifacts (argoproj#4579)

Signed-off-by: Daniel Herman <[email protected]>

fix(server): serve artifacts directly from disk to support large artifacts (argoproj#4589)

Signed-off-by: Daniel Herman <[email protected]>

fix(executor): Handle sidecar killing in a process-namespace-shared pod (argoproj#4575)

Signed-off-by: Daisuke Taniwaki <[email protected]>

docs: Add JSON schema for IDE validation (argoproj#4581)

Signed-off-by: Paul Brabban <[email protected]>

refactor: Use polling model for workflow phase metric (argoproj#4557)

Signed-off-by: Simon Behar <[email protected]>

Addressing reviewers comments

Signed-off-by: Alex Capras <[email protected]>

Addressing reviewers comments

docs: Minor typo fix (argoproj#4610)

Signed-off-by: Paavo Pokkinen <[email protected]>

fix(controller): Prevent tasks with names starting with digit to use either 'depends' or 'dependencies' (argoproj#4598)

Signed-off-by: terrytangyuan <[email protected]>

fix(docs): Bring minio chart instructions up to date (argoproj#4586)

Signed-off-by: Ranga Krishnan <[email protected]>

fix(executor): Fixed waitMainContainerStart returning prematurely. Closes argoproj#4599 (argoproj#4601)

Signed-off-by: fsiegmund <[email protected]>

feat(controller): Enhanced artifact repository ref. See argoproj#3184 (argoproj#4458)

Signed-off-by: Alex Collins <[email protected]>

fix: Null check pagination variable (argoproj#4617)

Signed-off-by: Simon Behar <[email protected]>

fix: Perform fields filtering server side (argoproj#4595)

Signed-off-by: Simon Behar <[email protected]>

fix(server): Correct webhook event payload marshalling. Fixes argoproj#4572 (argoproj#4594)

Signed-off-by: Alex Collins <[email protected]>

feat(ui): Add columns--narrower-height to AttributeRow (argoproj#4371)

fix: Fix TestCleanFieldsExclude (argoproj#4625)

Signed-off-by: Simon Behar <[email protected]>

fix(argo-server): fix global variable validation error with reversed dag.tasks (argoproj#4369)

Signed-off-by: chenyu.zheng <[email protected]>

fix: derive jsonschema and fix up issues, validate examples dir… (argoproj#4611)

Signed-off-by: Paul Brabban <[email protected]>

fix(ui): Reference secrets in EnvVars. Fixes argoproj#3973  (argoproj#4419)

Signed-off-by: Alejandro Tejera <[email protected]>

fix(ui): Fix Snyk issues (argoproj#4631)

Signed-off-by: Alex Collins <[email protected]>

feat(executor): More informative log when executors do not support output param from base image layer (argoproj#4620)

Signed-off-by: terrytangyuan <[email protected]>

Codegen patch. Signed off by [email protected]

Codegen patch. Signed off by [email protected]

Delete test.patch
alexcapras pushed a commit to alexcapras/argo that referenced this issue Dec 2, 2020
Signed-off-by: [email protected] <[email protected]>

feat(ui): Add Template/Cron workflow filter to workflow page. Closes argoproj#4532 (argoproj#4543)

Signed-off-by: Tianchu Zhao <[email protected]>

feat(executor): Auto create s3 bucket if not present.

Signed-off-by: Alex Capras <[email protected]>

Apply codegen

Signed-off-by: Alex Capras <[email protected]>

Add argo-e2e label to test wf

Signed-off-by: Alex Capras <[email protected]>

chore: Updated stress test YAML (argoproj#4569)

Signed-off-by: Alex Collins <[email protected]>

docs: Updated kubectl apply command in manifests README (argoproj#4577)

Signed-off-by: Stefan Gloutnikov <[email protected]>

feat(controller): Make MAX_OPERATION_TIME configurable. Close argoproj#4239 (argoproj#4562)

Signed-off-by: Alex Collins <[email protected]>

docs: Fix a typo in example (argoproj#4590)

Signed-off-by: Takayoshi Nishida <[email protected]>

feat(controller): Retry transient offload errors. Resolves argoproj#4464 (argoproj#4482)

Signed-off-by: Alex Collins <[email protected]>

fix(server): use the correct name when downloading artifacts (argoproj#4579)

Signed-off-by: Daniel Herman <[email protected]>

fix(server): serve artifacts directly from disk to support large artifacts (argoproj#4589)

Signed-off-by: Daniel Herman <[email protected]>

fix(executor): Handle sidecar killing in a process-namespace-shared pod (argoproj#4575)

Signed-off-by: Daisuke Taniwaki <[email protected]>

docs: Add JSON schema for IDE validation (argoproj#4581)

Signed-off-by: Paul Brabban <[email protected]>

refactor: Use polling model for workflow phase metric (argoproj#4557)

Signed-off-by: Simon Behar <[email protected]>

Addressing reviewers comments

Signed-off-by: Alex Capras <[email protected]>

Addressing reviewers comments

docs: Minor typo fix (argoproj#4610)

Signed-off-by: Paavo Pokkinen <[email protected]>

fix(controller): Prevent tasks with names starting with digit to use either 'depends' or 'dependencies' (argoproj#4598)

Signed-off-by: terrytangyuan <[email protected]>

fix(docs): Bring minio chart instructions up to date (argoproj#4586)

Signed-off-by: Ranga Krishnan <[email protected]>

fix(executor): Fixed waitMainContainerStart returning prematurely. Closes argoproj#4599 (argoproj#4601)

Signed-off-by: fsiegmund <[email protected]>

feat(controller): Enhanced artifact repository ref. See argoproj#3184 (argoproj#4458)

Signed-off-by: Alex Collins <[email protected]>

fix: Null check pagination variable (argoproj#4617)

Signed-off-by: Simon Behar <[email protected]>

fix: Perform fields filtering server side (argoproj#4595)

Signed-off-by: Simon Behar <[email protected]>

fix(server): Correct webhook event payload marshalling. Fixes argoproj#4572 (argoproj#4594)

Signed-off-by: Alex Collins <[email protected]>

feat(ui): Add columns--narrower-height to AttributeRow (argoproj#4371)

fix: Fix TestCleanFieldsExclude (argoproj#4625)

Signed-off-by: Simon Behar <[email protected]>

fix(argo-server): fix global variable validation error with reversed dag.tasks (argoproj#4369)

Signed-off-by: chenyu.zheng <[email protected]>

fix: derive jsonschema and fix up issues, validate examples dir… (argoproj#4611)

Signed-off-by: Paul Brabban <[email protected]>

fix(ui): Reference secrets in EnvVars. Fixes argoproj#3973  (argoproj#4419)

Signed-off-by: Alejandro Tejera <[email protected]>

fix(ui): Fix Snyk issues (argoproj#4631)

Signed-off-by: Alex Collins <[email protected]>

feat(executor): More informative log when executors do not support output param from base image layer (argoproj#4620)

Signed-off-by: terrytangyuan <[email protected]>

Codegen patch. Signed off by [email protected]

Codegen patch. Signed off by [email protected]

Delete test.patch

Signed-off-by: Alex Capras <[email protected]>
alexec added a commit that referenced this issue Dec 3, 2020
@alexec alexec added this to the v2.12 milestone Dec 3, 2020
@alexec
Copy link
Contributor

alexec commented Dec 3, 2020

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

Successfully merging a pull request may close this issue.

2 participants