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

add content id for attachments #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

farhan27
Copy link

Change attachments input from string to object to allow adding of attachment Id
This to allow the attachment ie image to be use in the email body
example of attachment input :

[{'filePath':'/test1', 'fileId':'test1'}]

Copy link
Contributor

@nmaludy nmaludy left a comment

Choose a reason for hiding this comment

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

Couple other things we need here:

  • Updated pack version. This is a breaking change, so a bump in the major version is probably necessary.
  • Updated changelog
  • Add unit tests

@@ -39,5 +39,5 @@
attachments:
type: "array"
items:
type: "string"
description: "The absolute paths to the files to be included as attachments."
type: "Object"
Copy link
Contributor

Choose a reason for hiding this comment

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

should "object" instead of uppercase "Object", this is why the test failed

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like simply converting this over, because it would break this action for all users and all workflows that are currently using this action and passing in strings here.

We should be able to use oneOf here to preserve backwards compatibility for existing users of this pack. Please do so.

    attachments:
      type: array
      description: |
        An array or list of items, either strings or objects/dictionaries, that specifies
        each file to be included as attachments.
      items:
        oneOf:  # this should be camelCase, as defined by JSONSchema
          - type: string
            description: The absolute path to the file to be attached
          - type: object
            description: An object containing file_id and file_path keys
            properties:
              file_id:
                type: string
                description: A unique identifier for the attachment in the mail envelope
              file_path:
                type: string
                description:  The absolute path to the file to be attached

Note that oneOf should be camelCase, as it is defined in JSONSchema, but file_id and file_path should both be snake_case, as they are interpreted by Python in StackStorm.

Comment on lines +47 to +48
filename = os.path.basename(el['filePath'])
with open(el['filePath'], 'rb') as f:
Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, it should be possible to make this backwards compatible, so users of this pack do not have to update every single place they call this action. Please modify the Python to use the old code path if el is a string, and the new codepath if el is a dictionary.

Also, please use snake_case for dictionary keys in StackStorm, not camelCase.

@@ -39,5 +39,5 @@
attachments:
type: "array"
items:
type: "string"
description: "The absolute paths to the files to be included as attachments."
type: "Object"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like simply converting this over, because it would break this action for all users and all workflows that are currently using this action and passing in strings here.

We should be able to use oneOf here to preserve backwards compatibility for existing users of this pack. Please do so.

    attachments:
      type: array
      description: |
        An array or list of items, either strings or objects/dictionaries, that specifies
        each file to be included as attachments.
      items:
        oneOf:  # this should be camelCase, as defined by JSONSchema
          - type: string
            description: The absolute path to the file to be attached
          - type: object
            description: An object containing file_id and file_path keys
            properties:
              file_id:
                type: string
                description: A unique identifier for the attachment in the mail envelope
              file_path:
                type: string
                description:  The absolute path to the file to be attached

Note that oneOf should be camelCase, as it is defined in JSONSchema, but file_id and file_path should both be snake_case, as they are interpreted by Python in StackStorm.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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

Successfully merging this pull request may close these issues.

4 participants