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

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

Merged
merged 67 commits into from
Apr 17, 2023
Merged

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

merged 67 commits into from
Apr 17, 2023

Conversation

joeyguerra
Copy link
Member

@joeyguerra joeyguerra commented Apr 13, 2023

Existing PRs and Issues

New from hubot-new/hubot fork

  • fix(package): Add package-lock.json generated with Node v14 / NPM v6 #32
  • fix(shell): Every other character is skipped when typing at the prompt with the default Shell adapter in Node.js versions > 16.19.0 #39
  • fix(actions): GitHub Actions for tests #45
  • chore(deps): bump lodash from 4.17.11 to 4.17.21 #47
  • chore(deps): bump minimatch and mocha #48
  • chore(deps): bump semver-regex and semantic-release #49
  • chore(deps): bump shelljs and standard #51
  • chore(deps): bump path-parse from 1.0.6 to 1.0.7 #54
  • chore(deps): bump hosted-git-info from 2.7.1 to 2.8.9 #55
  • chore(deps): bump handlebars from 4.1.0 to 4.7.7 #56
  • chore(deps): bump y18n from 4.0.0 to 4.0.3 #57
  • chore(deps): bump yargs-parser and nyc #58
  • chore(deps): bump express-basic-auth from 1.1.5 to 1.1.7 #59
  • fix(tests): Include CI for Windows, OSX #62

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

pearswj and others added 30 commits November 6, 2017 12:30
This is a fix for #1471. The
approach taken here is to compare the `HUBOT_HTTPD` environment variable
to the string `'false'` and then assign the value of this comparison to
the `enableHttpd` option. This ensures that a boolean value is always
passed to the robot constructor (which it already assumes).

The effect here is that setting `HUBOT_HTTPD` to `'false'` will disable
httpd, but all other values will enable it. The only backwards
compatibility issues caused by this change would be if someone happened
to already have `HUBOT_HTTPD=false` in their configuration; with this
new change they would find that httpd would actually be disabled now.
fix: address TypeError by experss
docs(windows): Fix setx typo to set and put double quotes in the corr…
docs(patterns): use scoped npm package as adapter
Add `package-lock.json` generated with Node v14 / NPM v6
fixes #7. Align to new org and maintainers.
Bumps [lodash](https://github.com/lodash/lodash) from 4.17.11 to 4.17.21.
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.17.11...4.17.21)

---
updated-dependencies:
- dependency-name: lodash
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [minimatch](https://github.com/isaacs/minimatch) to 3.1.2 and updates ancestor dependency [mocha](https://github.com/mochajs/mocha). These dependencies need to be updated together.


Updates `minimatch` from 3.0.4 to 3.1.2
- [Release notes](https://github.com/isaacs/minimatch/releases)
- [Changelog](https://github.com/isaacs/minimatch/blob/main/changelog.md)
- [Commits](isaacs/minimatch@v3.0.4...v3.1.2)

Updates `mocha` from 5.2.0 to 10.2.0
- [Release notes](https://github.com/mochajs/mocha/releases)
- [Changelog](https://github.com/mochajs/mocha/blob/master/CHANGELOG.md)
- [Commits](mochajs/mocha@v5.2.0...v10.2.0)

---
updated-dependencies:
- dependency-name: minimatch
  dependency-type: indirect
- dependency-name: mocha
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <support@github.com>
Removes [shelljs](https://github.com/shelljs/shelljs). It's no longer used after updating ancestor dependency [standard](https://github.com/standard/standard). These dependencies need to be updated together.


Removes `shelljs`

Updates `standard` from 10.0.3 to 17.0.0
- [Release notes](https://github.com/standard/standard/releases)
- [Changelog](https://github.com/standard/standard/blob/master/CHANGELOG.md)
- [Commits](standard/standard@v10.0.3...v17.0.0)

---
updated-dependencies:
- dependency-name: shelljs
  dependency-type: indirect
- dependency-name: standard
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <support@github.com>
…4.17.21

chore(deps): bump lodash from 4.17.11 to 4.17.21
…ch-and-mocha-3.1.2

chore(deps): bump minimatch and mocha
@xurizaemon
Copy link
Collaborator

xurizaemon commented Apr 14, 2023

@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
Copy link
Member Author

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

@joeyguerra joeyguerra changed the title 2023 catchup fix: #1541 activity on repo has been stalled, this kickstarts it Apr 14, 2023
@joeyguerra joeyguerra changed the title fix: #1541 activity on repo has been stalled, this kickstarts it fix: activity on repo has been stalled, this kickstarts it #1541 Apr 14, 2023
@xurizaemon
Copy link
Collaborator

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
Copy link
Member Author

joeyguerra commented Apr 14, 2023

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
Copy link
Member

technicalpickles commented Apr 14, 2023

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.

Copy link
Member

@technicalpickles technicalpickles left a comment

Choose a reason for hiding this comment

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

Gave this a test drive on hubot-for-hubot locally. The local coffeescripts worked fine. I was having problems using external-scripts.json though, but that seems to be more about using npm link or npm install ../hubot than the PR.

I think we can call this good! Is there anything else to fix/update before merging? Can always do it in followups too.

mixture of multiple fixes, features, doc updates?

I'd lean towards feature, ie minor version.

@@ -352,7 +352,7 @@ class Robot {
const full = path.join(filepath, path.basename(filename, ext))

// see https://github.com/hubotio/hubot/issues/1355
if (!require.extensions[ext]) { // eslint-disable-line
Copy link
Member

Choose a reason for hiding this comment

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

Confirmed this works w/ coffee scripts.

@@ -0,0 +1,282 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking a comment at the top of the file. Would be good to move off this, but will be a pain because of how much plugins depend on it 😒

src/robot.js Outdated
@@ -352,7 +352,7 @@ class Robot {
const full = path.join(filepath, path.basename(filename, ext))

// see https://github.com/hubotio/hubot/issues/1355
if (!require.extensions[ext]) { // eslint-disable-line
if (['.js', '.mjs'].indexOf(ext) == -1) { // eslint-disable-line
Copy link
Member

Choose a reason for hiding this comment

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

It's more that it's a hard-coded list, and there could be other extension types people register outside the context of hubot.

Based on my reading of the docs, the goal should be to transpile to javascript instead, so it becomes moot.

@joeyguerra
Copy link
Member Author

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 joeyguerra changed the title fix: activity on repo has been stalled, this kickstarts it #1541 feat: activity on repo has been stalled, this kickstarts it #1541 Apr 15, 2023
@joeyguerra
Copy link
Member Author

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
Copy link
Member

technicalpickles commented Apr 17, 2023

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
Copy link
Member Author

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

@xurizaemon
Copy link
Collaborator

xurizaemon commented Apr 17, 2023

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
Copy link
Collaborator

xurizaemon commented Apr 17, 2023

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
Copy link
Member Author

@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
Copy link
Member

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
Copy link
Member

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

I'm going to:

  • merge
  • add y'all as maintainers
  • see if I can release a one-off w/o using semantic release

@technicalpickles technicalpickles merged commit a1af4c8 into hubotio:master Apr 17, 2023
@joeyguerra joeyguerra deleted the 2023-catchup branch April 17, 2023 15:54
@joeyguerra
Copy link
Member Author

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

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.

Can we please get rid of the SIGTERM catch?
8 participants