-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
docs/tutorials/toy-connector.md
Outdated
# 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 |
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 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:
- not wrapping these
- 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.
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.
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
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 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.
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.
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.
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.
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
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