-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: add github gist publisher #222
feat: add github gist publisher #222
Conversation
8a1e6db
to
29889fe
Compare
9f76780
to
d79c195
Compare
Looks good, just found one weird issue. When |
You're right. Files within a gist are identified by their names. If name changes, it's going to be treated a different file. I guess we can read the gist before update and mark the old file for deletion. I'll update. Thanks |
6b82be6
to
976a51f
Compare
if !ok { | ||
return | ||
} | ||
documentName := metaMap.(plugin.MapData)["name"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the wrong name to use - the name in the meta block is the template's name, not the document name. We should use the name of the document
block (I think it is available in the document map?) -- "example" in the snippet above. Document name should always be present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for your contribution
Secret: true, | ||
}, | ||
}, | ||
Args: dataspec.ObjectSpec{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you can add some constraints/default values? This helps with our documentation generation (not yet done for publishers, but would be in future) and allows you to skip repetetive calls like checks for Nulls in your code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Andrew-Morozko As mentioned in the issue here #134, these arguments are optional. so non null constraint doesn't make sense?. We can add a default for make_public
. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}, | ||
} | ||
// overrides params if defined | ||
descriptionAttr := params.Args.GetAttr("description") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, making description constraint.NonNull
and cty.StringVal("")
by default can simplify the code below
Co-authored-by: Andrew Morozko <andrewmorozko@gmail.com>
3c490f6
to
d600083
Compare
closes #134