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

Hide connector scaffolding behind a script #2080

Merged
merged 4 commits into from
Feb 17, 2021
Merged

Conversation

michel-tricot
Copy link
Contributor

@michel-tricot michel-tricot commented Feb 16, 2021

What

Hide connector scaffolding behind a script. Helps with #725

How

Add a new command to the manage.sh script. I had to rename the scaffold names so they can be passed in as argument without breaking

@michel-tricot michel-tricot added this to the 2021/02/19 milestone Feb 16, 2021
# Install NPM from https://www.npmjs.com/get-npm if you don't have it
$ npm install
$ npm run generate
$ ./tools/integrations/manage.sh scaffold
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think this is a DX friendly choice. the manage.sh script is intimidating and filled with all sorts of deploy / publishing logic. wrapping 2 very common place commands that every developer has used before inside a really huge script doesn't seem great from the DX point of view. trying to put myself in the shoes of a contributor who is coming to our project for the first time, i would probably not be willing to debug an error in manage.sh, but if either npm install or npm run generate failed, i would at least try to debug it.

i would suggest:

  1. not wrapping these
  2. if you do think you need a script, put it in a separate script that is just for this case and not full of other stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

is a contributor expected to use this script? If yes then I agree but if not and this is for CI purposes then I'm fine with it

Copy link
Contributor

Choose a reason for hiding this comment

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

my interpretation was that it was used by the CI and a contributor. If I am misunderstanding and the contributor does not need to interact with this then I am ambivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one of the reason I had in mind when hiding it in a script was to not duplicate the generation commands in many places (docs, gradle, tutorials...).

I'll just revert the contributor piece but I will keep the command in the script so it is easier to invoke from gradle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried to inspire myself from rails where you have one super command that can do a lot of things, and everything is discoverable through that command

@michel-tricot michel-tricot merged commit ac092ad into master Feb 17, 2021
@michel-tricot michel-tricot deleted the mt-scaffold branch February 17, 2021 04:09
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.

3 participants