-
Notifications
You must be signed in to change notification settings - Fork 64
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
Create api samples #421
Create api samples #421
Conversation
…into create-api-samples
…into create-api-samples
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.
Great to see this. Are we planning to add additional API sample projects before merging this? If not, I think we should remove the prompt flow.
I also think we should add some info to the README on the need to create a developer account and an app in order to obtain a client id and secret.
packages/cli/lib/configs.js
Outdated
const { logErrorInstance } = require('@hubspot/cli-lib/errorHandlers'); | ||
|
||
// https://developer.github.com/v3/#user-agent-required | ||
const USER_AGENT_HEADERS = { 'User-Agent': 'HubSpot/hubspot-cms-tools' }; |
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 think we should create a constant in packages/cli-lib/http/requestOptions
for the constant and share it between here and getRequestOptions
.
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.
Nice point. Created DEFAULT_USER_AGENT_HEADERS constant, and shared it to projects and github.js
packages/cli/lib/configs.js
Outdated
* @param {String} repoName - name of the github repository | ||
* @returns {Buffer|Null} Zip data buffer | ||
*/ | ||
async function downloadConfig(repoName, filePath) { |
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.
While this works, it does seem like a pretty indirect way to get a JSON file with information on the different samples.
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.
Simplified the logic of getting configs to 1 method, and made it more abstract for getting json files.
packages/cli/commands/create.js
Outdated
if (fs.existsSync(filePath)) { | ||
const { overwrite } = await overwriteSamplePrompt(filePath); | ||
if (overwrite) { | ||
const removingSpinner = ora(`Removing existing ${filePath} folder`); |
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.
Since we're dealing with a local file system operation, is this necessary?
case TYPES['api-sample']: { | ||
const filePath = path.join(dest, name); | ||
if (fs.existsSync(filePath)) { | ||
const { overwrite } = await overwriteSamplePrompt(filePath); |
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.
This is a neat idea! We should consider using it in other hs create
flows.
if (!sampleType || !sampleLanguage) { | ||
logger.error( | ||
`Currently there are no samples available, please, try again later.` | ||
); |
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.
Should the message here be different?
…i-sample from help
Description and Context
This is the first PR about integration with SDK sample-apps.
Currently initial flow is the following:
hs create api-sample
to check, which samples are currently available on which languages.We've added one repo with first oauth sample app on node language to test .
Screenshots