search

hubotio / hubot

A customizable life embetterment robot.
https://hubotio.github.io/hubot/
MIT License
16.66k stars 3.75k forks source link

feat: activity on repo has been stalled, this kickstarts it #1541 #1581

Closed joeyguerra closed 1 year ago

joeyguerra commented 1 year ago

Existing PRs and Issues

New from hubot-new/hubot fork

Closes #1403 , #1405 , #1470 , #1483, #1485, #1509, #1530, #1546, #1548, #1564

xurizaemon commented 1 year ago

@technicalpickles @joeyguerra I think we should see Github Actions kick in on this MR - does that need to be enabled on the repo?

Is there a task also to remove the Travis CI checks as well, if they're sufficiently replaced by Github Actions here? They didn't show up on hubot-new/hubot but we wouldn't have had the expected configuration in place.

xurizaemon commented 1 year ago

https://github.com/hubotio/hubot/pull/1581/commits/3ceeb0a4881270ecc9d134e25aaf1806f0236526 may want to be revisited - that change can be updated to retain the existing org.

xurizaemon commented 1 year ago

Whooo, this PR adds 10K LoC. Is that all from .coffee > .js?!

joeyguerra commented 1 year ago

It's the package-lock.json file. 9,494 LOC.

technicalpickles commented 1 year ago

does that need to be enabled on the repo?

It is already enabled 🤔 They do require an initial approval for first time contributors, but I am not seeing a way to enable them. Will see if I can figure it out.

technicalpickles commented 1 year ago

I have no idea why I can't approve or run GitHub Actions for this branch (followed the docs and there's not an approve workflow as suggested).

I was able to npm install and npm test locally on macOS w/ Node 18.x. Two caveats though.

1) npm install had some warnings about an old lockfile. May want to npm install and commit the updated package.json at some point (doesn't need to be now)

npm WARN old lockfile
npm WARN old lockfile The package-lock.json file was created with an old version of npm,
npm WARN old lockfile so supplemental metadata must be fetched from the registry.
npm WARN old lockfile
npm WARN old lockfile This is a one-time fix-up, please be patient...
npm WARN old lockfile

2) npm test is successful, but seems to hang after printing out the summary:


load: 7.47  cmd: node 44076 waiting 1.04u 0.12s
load: 7.47  cmd: node 44076 waiting 1.04u 0.12s
load: 7.47  cmd: node 44076 waiting 1.04u 0.12s
^C----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------|---------|----------|---------|---------|-------------------
All files |       0 |        0 |       0 |       0 |
----------|---------|----------|---------|---------|-------------------

Looks like it may be nyc which is doing... coverage reporting? I'm not very worried about this, but should be fixed up as a followup.

xurizaemon commented 1 year ago

That "pending" thing was raised in https://github.com/hubot-new/hubot/issues/33 which is closed but IDK if the issue was resolved.

Is it that Travis failing prevents Github Actions from proceeding perhaps?

technicalpickles commented 1 year ago

Is it that Travis failing prevents Github Actions from proceeding perhaps?

Doubtful. Looking more closely, it's a branch protection for the default branch, and 'waiting for status to be reported' suggests to me it's not getting kicked off at all. I can remove that requirement for now.

technicalpickles commented 1 year ago

Something I'd like to double check before approving & merging is trying to upgrade an existing hubot. https://github.com/hubotio/hubot-for-hubot is one we can use to test. I'm more worried about loading and running the tests than anything else, so testing with the shell adapter is fine instead of getting it hooked back up to the hubotio slack.

xurizaemon commented 1 year ago

@joeyguerra don't forget the commit message syntax in https://github.com/hubotio/hubot/blob/master/CONTRIBUTING.md for semantic-release. (I'll eventually add a workflow to ensure that, but not here. Previously discussed @ https://github.com/hubot-new/hubot/issues/44.)

joeyguerra commented 1 year ago

@xurizaemon any suggestions on what to put for the title since this PR includes a mixture of multiple fixes, features, doc updates?

xurizaemon commented 1 year ago

Uhmm ... C-C-C-C-C-C-COMBO? Not sure about the issue title sorry.

AFAIK it's commit messages that semantic-release would care about, and it's likely to use the individual branch commits rather than the merge commits. Apologies if I didn't make my thinking explicit enough when suggesting this a couple days ago!

joeyguerra commented 1 year ago

Ah. Now I see what you're saying. I was thinking, "surely we don't have to go back and update every commit message". But, in fact, you're saying we do. But those individual commits were not "atomic" in that they encapsulated a complete thought or change. Any suggestions for that situation?

btw, thank you for your patience. It’s one thing to write down my thoughts in chat or email to someone I work with often. It’s quite another level of skill to communicate thoughts and ideas when face to face comma is missing. Wish I would’ve paid more attention in my writing classes.

technicalpickles commented 1 year ago

The upside of travis not working is semantic release commits won't be enforced. I can also bypass it as a maintainer even if it did.

As a followup, using PR titles instead of commits would be more practical and easier to manage instead of each commit.

joeyguerra commented 1 year ago

When testing with hubot-for-hubot, http client bugs were exposed. buildOptions (extend) isn’t working as expected. I pushed a fix to this PRs branch. I’ll test it again with hubot-for-hubot later this evening.

joeyguerra commented 1 year ago

I'm done with my testing as well. I pushed some bug fixes to the httpclient module after testing with hubot-for-hubot. @technicalpickles good call! Thank you.

@technicalpickles do you know how to do a dry run semantic-release so we can see how the commit messages are going to look? I was wondering if we should create a different branch, put all the changes that are in this PR into that new one and start from there instead to shore up the commit messages, but I don't know how semantic-release builds the change log from the commit messages. The documentation says it parses the commit messages between the previous release (git tag) and the new one. But I'm still unclear what commit messages it's going to get.

technicalpickles commented 1 year ago

do you know how to do a dry run semantic-release so we can see how the commit messages are going to look?

I'm not super familiar, as I wasn't the main proponent of using this. I pulled it down locally, and looks like there is a --dry-run command at least. It looks like it needs some GitHub and/or NPM tokens to run.

The documentation says it parses the commit messages between the previous release (git tag) and the new one. But I'm still unclear what commit messages it's going to get.

My hunch is that it wouldn't even run, because it was part of Travis CI which isn't running. I'm fine with doing this release manually if needed though.

joeyguerra commented 1 year ago

Ok. I'm ready whenever you are :) What are the next steps?

xurizaemon commented 1 year ago

FWIW - I proposed (https://github.com/hubot-new/hubot/issues/44) we adopt semantic-release pattern based on seeing it documented in CONTRIBUTING.md here.

I supported that approach because it's easier as a distributed team to have a clear process, and because adopting an existing established (apparently!!) process would make more sense for community adoption than coming up with a new one, or (worse) forgetting to communicate the changes to users clearly.

It seems low-maintenance which I think is good ... But on testing, it feels a little opaque - I tried a few combos just now and couldn't work out how to get it to simulate generating release notes. As @technicalpickles saw even for --dry-run it wants NPM and Github tokens, and I would be reluctant to give it those without seeing the output of --dry-run first!

OTOH if there's a better method, we're free to do that too! So this comment is mostly to say we can be flexible on it :)

xurizaemon commented 1 year ago

I'm still unclear what commit messages it's going to get.

I would expect it to get all the commits between the most recent NPM release and master at time of the release.

You can see this from the output of: git log --pretty=short v3.3.2..2023-catchup (source branch here instead of master, as we're trying to see what would be the state of things; if there were other PRs to consider, that would factor in too).

PS. @joeyguerra we can totally rework those commit messages without needing a new branch/PR for it. Happy to show how or do this if it's helpful.

joeyguerra commented 1 year ago

@xurizaemon yes please, I'd appreciate you showing me how to rework those commit messages.

@technicalpickles when you say, "doing the release manually", do you mean execute the semantic-release cli command from your local machine to publish an updated package to NPM? Or adding the semantic-release command to one of (or create a new one) the GitHub Actions workflow yaml file?

technicalpickles commented 1 year ago

when you say, "doing the release manually", do you mean execute the semantic-release cli command from your local machine to publish an updated package to NPM? Or adding the semantic-release command to one of (or create a new one) the GitHub Actions workflow yaml file?

I mean publishing with the npm command, and tagging git. I can create new "Release" really easily, because it can figure most of that out automatically nowadays (that didn't exist when semantic-release was being used)

technicalpickles commented 1 year ago

Ok. I'm ready whenever you are :) What are the next steps?

I'm going to:

joeyguerra commented 1 year ago

I'm trying a dry run with semantic-release and running into errors. First one is "can't login with password" :(