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

feature: allow bots to have hyphens in google cloud functions #218

Closed
wants to merge 6 commits into from
Closed

feature: allow bots to have hyphens in google cloud functions #218

wants to merge 6 commits into from

Conversation

sofisl
Copy link
Contributor

@sofisl sofisl commented Jan 10, 2020

Related to Issue #177: after doing some digging with @bcoe, we found out that you can publish a Google Cloud function with a hyphen. I'm recommending we make the change in this PR to provide naming consistency in the bots we create and deploy to GCF. In generate-bot, I've changed the logic so that module.exports can export a function with a hyphen, like so:

import` { GCFBootstrapper } from 'gcf-utils';
import appFn from './{{programName}}';

const bootstrap = new GCFBootstrapper();
module.exports['{{program-name}}']= bootstrap.gcf(appFn);

`

which should work.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 10, 2020
@sofisl sofisl requested a review from bcoe January 10, 2020 01:15
@sofisl sofisl requested a review from orthros January 10, 2020 01:18
@orthros
Copy link
Contributor

orthros commented Jan 14, 2020

Thanks for doing this!

Just for my own clarification: this change (which removes the replacement of '-' with '_' in folder names) will make it so that future bots will be able to export their functions in the manner you provided above?

As far as I can tell, this would break other bots like conventional-commit-lint as there is now a discrepancy between the folder name and the exported function name, correct?

@@ -43,9 +43,6 @@ for f in *; do
(
cd "$f" || exit 1
echo "$f"
# Javascript does not allow function names with '-' so we need to
# replace the directory name (which might have '-') with "_"
functionname=${f//-/_}
Copy link
Contributor

Choose a reason for hiding this comment

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

Without functionname set, this will cause the gcloud functions deploy "$functionname"... to fail a few lines down, perhaps replace with functionname=${f}?

Copy link
Contributor

@orthros orthros left a comment

Choose a reason for hiding this comment

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

As in my comment, thank you for working on making this a much easier to use repo!

As far as I can tell, this would break other bots like conventional-commit-lint as there is now a discrepancy between the folder name (and therefore the function's name) and the exported function name, correct?

@bcoe
Copy link
Contributor

bcoe commented Jan 15, 2020

@sofisl @orthros it sounds like we'd want to move these legacy functions to a folder that uses _ before we land this? or perhaps change the remote folders and key names accordingly?

@orthros
Copy link
Contributor

orthros commented Jan 15, 2020

@bcoe Either that or we move everything at once (fixing the function names in the process)

The problem with that is it would change the URL that the bots can be found at, and would then necessitate someone to fix the GitHub Application to point to the correct URL

@bcoe bcoe added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 15, 2020
@bcoe
Copy link
Contributor

bcoe commented Jan 15, 2020

@sofisl I think we're going to need to figure out a migration strategy for the culprit bots before we land this, or we're going to break quite a few things.

@bcoe
Copy link
Contributor

bcoe commented Jan 22, 2020

looking at the amount of work it would take to migrate all of our existing bots, and the benefit we'd get, I think we might do better @sofisl to move the logic for replacing - with _ into generate-bot, rather than removing it from @orthros' scripts (at least for the time being)...

@bcoe
Copy link
Contributor

bcoe commented Jan 28, 2020

closing with tracking issue: #242

@bcoe bcoe closed this Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants