-
Notifications
You must be signed in to change notification settings - Fork 6
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: create notes using HackMD #30
Conversation
Passing CI on my fork: https://github.com/mmarchini/meet/runs/964840886?check_suite_focus=true |
cc @wesleytodd :) |
lib/notes.js
Outdated
const resp = await client.repos.getContents({ | ||
owner: opts.owner, | ||
repo: opts.repo, | ||
path: `.github/ISSUE_TEMPLATE/${opts.notesTemplate || 'meeting-notes.md'}` |
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.
I think we should move this to .github/
right? Since it is technically not an issue template?
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.
I would say we may want to have our own folder in .github/
- perhaps .github/meet/
. But yes, I agree with this.
run.js
Outdated
@@ -14,8 +16,10 @@ const defaultTemplate = require('./lib/default-template') | |||
const labels = list(core.getInput('labels')) | |||
const issueTemplate = core.getInput('issueTemplate') | |||
const createWithin = core.getInput('createWithin') | |||
const agendaLabel = core.getInput('agendaLabel') | |||
const agendaLabel = core.getInput('agendaLabel') || 'meeting-agenda' |
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.
I think you can set this as a default for the action instead of here, right? The benefit being it would show in the action docs.
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.
Hm, I see it is set as a default above. Are both necessary?
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.
No idea, this was my first time working on an actual Action (instead of consuming one), so I wanted to be extra safe 😅. It's probably not necessary though.
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.
Yep, setting it as the default in action.yml
would be ideal.
cc: @pkgjs/meet-collaborators |
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.
Pending comments, huge +1. Thank you for this work, Mary ❤️
if I can be of any help getting this to land (or developing it further), please let me know!
|
+1 @boneskull |
@mmarchini any update on this? ❤️ |
b088fdb
to
80b87bf
Compare
Addressed all inline comments and rebased.
I think that's already how it works, but if it's not we should do it in a separate PR and include issue templating as well
That sounds interesting, let's open an issue to investigate in the future
<3 |
Test on my fork: https://github.com/mmarchini/meet/pull/15/checks?check_run_id=1387198907 Seems like I messed something up and now the issue template is not being used 🤔 |
dc97e91
to
e04b9cb
Compare
Ok, should be working now. Test on my fork: https://github.com/mmarchini/meet/runs/1387488797?check_suite_focus=true |
date, | ||
agendaLabel: opts.agendaLabel, | ||
agendaIssues: opts.agendaIssues, | ||
labels: opts.labels, |
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 will need to change with #39
No description provided.