-
-
Notifications
You must be signed in to change notification settings - Fork 43
feat: updated, refactored and cleaned up #180
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
base: main
Are you sure you want to change the base?
Conversation
I think it would be good to get the updated version working/tested in real life before landing changes to the repo. I won't have time to debug if changes break the live tooling which regularly updates from this repo to pull in changes. |
+1. I haven't tested this for real yet, so we shouldn't land before I'm able to confirm it works fine. Would unit testing also make sense here? |
Adding HackMD docs as we're closing a partnership with'em. |
cc @avivkeller ☝️ |
console.log(`Running meeting artifacts creation for group: ${group}`); | ||
|
||
// Spawn the npm script with proper stdio inheritance | ||
const child = spawn('npm', ['run', scriptName], { |
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.
Wouldn't it make more sense to not rely on npm
here? That way, this can be triggered via npx
, and not rely on the local package.json
containing the scripts (See #179)
Or, explicitly set the cwd
to tell npm
where the package.json
is.
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.
My brain is tired can you make a code suggestion?
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.
Actually, you can delete this whole file and just make create-node-meeting-artifacts.mjs
the cli entrypoint.
This file is basically calling create-node-meeting-artifacts.mjs [the input passed]
FYI this PR staled a bit as I'm going to work next weekend on (Aiming the Sunday or Saturday) (16th/17th of August) |
cc @mhdawson this PR is ready to be reviewed and merged once we set the tokens 🙇 |
HACKMD_API_TOKEN: ${{ secrets.HACKMD_API_TOKEN }} | ||
HACKMD_TEAM_NAME: ${{ secrets.HACKMD_TEAM_NAME }} | ||
GOOGLE_API_KEY: ${{ secrets.GOOGLE_API_KEY }} | ||
run: node create-node-meeting-artifacts.mjs ${{ github.event.inputs.meeting_group }} |
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.
How will this work on schedule? TSC?
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.
Hmm?
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.
You have a schedule trigger on this workflow. When that trigger occurs, ${{ github.event.inputs.meeting_group }}
will be blank. Will it default to TSC, like it does here?
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 it won't, good question, can I add fields for manual workflow triggers?
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.
Bump, @avivkeller
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 don't understand your question. I think if you just remove the schedule trigger, and make this only run manually, it'll be fine
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.
My question is, if we support manual triggers, can we have an input field that allows us to set this? Otherwise I'll remove manual dispatch, as this should be pretty much cron-based.
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.
In the event of a manual trigger, yes, you can define inputs. In the event of a schedule trigger, no, you cannot define inputs.
Why would a schedule trigger even be needed in this repository, shouldn't that be handled by the repos relying on this, not this repo itself?
@bensternthal / @ryanaslett I'm ready to set the tokens once openjs-foundation/infrastructure#78 gets done. |
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.
The remaining comments of mine are nitpicks.
LGTM, thank you, Claudio!
@@ -2,28 +2,79 @@ | |||
"name": "create-node-meeting-artifacts", | |||
"version": "0.0.1", | |||
"description": "Creates issue and minutes doc for node meeting", | |||
"main": "index.js", | |||
"type": "module", | |||
"main": "create-node-meeting-artifacts.mjs", | |||
"bin": { | |||
"create-meeting": "./bin/create-meeting" |
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.
"create-meeting": "./bin/create-meeting" | |
"create-meeting": "./create-node-meeting-artifacts.mjs" |
"uvwasi-meeting": "node create-node-meeting-artifacts.mjs uvwasi", | ||
"uvwasi-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs uvwasi", | ||
"tsc-meeting": "node create-node-meeting-artifacts.mjs tsc", | ||
"tsc-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs tsc", | ||
"build-meeting": "node create-node-meeting-artifacts.mjs build", | ||
"build-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs build", | ||
"diag-meeting": "node create-node-meeting-artifacts.mjs diag", | ||
"diag-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs diag", | ||
"diag-deepdive-meeting": "node create-node-meeting-artifacts.mjs diag_deepdive", | ||
"diag-deepdive-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs diag_deepdive", | ||
"typescript-meeting": "node create-node-meeting-artifacts.mjs typescript", | ||
"typescript-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs typescript", | ||
"release-meeting": "node create-node-meeting-artifacts.mjs Release", | ||
"release-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs Release", | ||
"cross-project-council-meeting": "node create-node-meeting-artifacts.mjs cross_project_council", | ||
"cross-project-council-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs cross_project_council", | ||
"modules-meeting": "node create-node-meeting-artifacts.mjs modules", | ||
"modules-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs modules", | ||
"tooling-meeting": "node create-node-meeting-artifacts.mjs tooling", | ||
"tooling-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs tooling", | ||
"security-wg-meeting": "node create-node-meeting-artifacts.mjs security-wg", | ||
"security-wg-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs security-wg", | ||
"next-10-meeting": "node create-node-meeting-artifacts.mjs next-10", | ||
"next-10-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs next-10", | ||
"package-maintenance-meeting": "node create-node-meeting-artifacts.mjs package-maintenance", | ||
"package-maintenance-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs package-maintenance", | ||
"package-metadata-interop-meeting": "node create-node-meeting-artifacts.mjs package_metadata_interop", | ||
"package-metadata-interop-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs package_metadata_interop", | ||
"ecosystem-report-meeting": "node create-node-meeting-artifacts.mjs ecosystem_report", | ||
"ecosystem-report-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs ecosystem_report", | ||
"sustainability-collab-meeting": "node create-node-meeting-artifacts.mjs sustainability_collab", | ||
"sustainability-collab-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs sustainability_collab", | ||
"standards-meeting": "node create-node-meeting-artifacts.mjs standards", | ||
"standards-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs standards", | ||
"security-collab-meeting": "node create-node-meeting-artifacts.mjs security_collab", | ||
"security-collab-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs security_collab", | ||
"loaders-meeting": "node create-node-meeting-artifacts.mjs loaders", | ||
"loaders-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs loaders", | ||
"web-server-frameworks-meeting": "node create-node-meeting-artifacts.mjs web-server-frameworks", | ||
"web-server-frameworks-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs web-server-frameworks" |
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.
Assuming that https://github.com/nodejs/create-node-meeting-artifacts/pull/180/files#r2286679090 is accepted:
"uvwasi-meeting": "node create-node-meeting-artifacts.mjs uvwasi", | |
"uvwasi-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs uvwasi", | |
"tsc-meeting": "node create-node-meeting-artifacts.mjs tsc", | |
"tsc-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs tsc", | |
"build-meeting": "node create-node-meeting-artifacts.mjs build", | |
"build-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs build", | |
"diag-meeting": "node create-node-meeting-artifacts.mjs diag", | |
"diag-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs diag", | |
"diag-deepdive-meeting": "node create-node-meeting-artifacts.mjs diag_deepdive", | |
"diag-deepdive-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs diag_deepdive", | |
"typescript-meeting": "node create-node-meeting-artifacts.mjs typescript", | |
"typescript-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs typescript", | |
"release-meeting": "node create-node-meeting-artifacts.mjs Release", | |
"release-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs Release", | |
"cross-project-council-meeting": "node create-node-meeting-artifacts.mjs cross_project_council", | |
"cross-project-council-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs cross_project_council", | |
"modules-meeting": "node create-node-meeting-artifacts.mjs modules", | |
"modules-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs modules", | |
"tooling-meeting": "node create-node-meeting-artifacts.mjs tooling", | |
"tooling-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs tooling", | |
"security-wg-meeting": "node create-node-meeting-artifacts.mjs security-wg", | |
"security-wg-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs security-wg", | |
"next-10-meeting": "node create-node-meeting-artifacts.mjs next-10", | |
"next-10-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs next-10", | |
"package-maintenance-meeting": "node create-node-meeting-artifacts.mjs package-maintenance", | |
"package-maintenance-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs package-maintenance", | |
"package-metadata-interop-meeting": "node create-node-meeting-artifacts.mjs package_metadata_interop", | |
"package-metadata-interop-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs package_metadata_interop", | |
"ecosystem-report-meeting": "node create-node-meeting-artifacts.mjs ecosystem_report", | |
"ecosystem-report-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs ecosystem_report", | |
"sustainability-collab-meeting": "node create-node-meeting-artifacts.mjs sustainability_collab", | |
"sustainability-collab-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs sustainability_collab", | |
"standards-meeting": "node create-node-meeting-artifacts.mjs standards", | |
"standards-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs standards", | |
"security-collab-meeting": "node create-node-meeting-artifacts.mjs security_collab", | |
"security-collab-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs security_collab", | |
"loaders-meeting": "node create-node-meeting-artifacts.mjs loaders", | |
"loaders-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs loaders", | |
"web-server-frameworks-meeting": "node create-node-meeting-artifacts.mjs web-server-frameworks", | |
"web-server-frameworks-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs web-server-frameworks" | |
"uvwasi-meeting": "create-meeting uvwasi", | |
"uvwasi-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs uvwasi", | |
"tsc-meeting": "create-meeting tsc", | |
"tsc-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs tsc", | |
"build-meeting": "create-meeting build", | |
"build-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs build", | |
"diag-meeting": "create-meeting diag", | |
"diag-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs diag", | |
"diag-deepdive-meeting": "create-meeting diag_deepdive", | |
"diag-deepdive-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs diag_deepdive", | |
"typescript-meeting": "create-meeting typescript", | |
"typescript-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs typescript", | |
"release-meeting": "create-meeting Release", | |
"release-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs Release", | |
"cross-project-council-meeting": "create-meeting cross_project_council", | |
"cross-project-council-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs cross_project_council", | |
"modules-meeting": "create-meeting modules", | |
"modules-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs modules", | |
"tooling-meeting": "create-meeting tooling", | |
"tooling-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs tooling", | |
"security-wg-meeting": "create-meeting security-wg", | |
"security-wg-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs security-wg", | |
"next-10-meeting": "create-meeting next-10", | |
"next-10-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs next-10", | |
"package-maintenance-meeting": "create-meeting package-maintenance", | |
"package-maintenance-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs package-maintenance", | |
"package-metadata-interop-meeting": "create-meeting package_metadata_interop", | |
"package-metadata-interop-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs package_metadata_interop", | |
"ecosystem-report-meeting": "create-meeting ecosystem_report", | |
"ecosystem-report-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs ecosystem_report", | |
"sustainability-collab-meeting": "create-meeting sustainability_collab", | |
"sustainability-collab-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs sustainability_collab", | |
"standards-meeting": "create-meeting standards", | |
"standards-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs standards", | |
"security-collab-meeting": "create-meeting security_collab", | |
"security-collab-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs security_collab", | |
"loaders-meeting": "create-meeting loaders", | |
"loaders-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs loaders", | |
"web-server-frameworks-meeting": "create-meeting web-server-frameworks", | |
"web-server-frameworks-meeting:dev": "node --env-file=.env create-node-meeting-artifacts.mjs web-server-frameworks" |
This PR removes the usage of very old dependencies for the ones maintained officially by Google with updated APIs.
This PR refactors and modularizes the source code and ensures it is functional via environmnent-based configuration, instead of fie-system ones.
It also amplifies the code, documentation and ensures it is easy to contribute towards.