-
Notifications
You must be signed in to change notification settings - Fork 816
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
Headless Init and Configure #371
Conversation
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 changes to 'packages/amplify-frontend-ios' is missing completly in this PR.
const amplifyRC = JSON.parse(fs.readFileSync(amplifyRCFilePath, 'utf8')); | ||
result = amplifyRC[PublishIgnoreRCLabel]; | ||
try { | ||
const amplifyRC = require(amplifyRCFilePath); |
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.
We're getting rid of the amplifyRC file with the new multi-env design. Need a strategy to take this ignore publish into account.
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.
done.
const amplifyRC = require(amplifyRCFilePath); | ||
result = amplifyRC[PublishIgnoreRCLabel]; | ||
} catch (e) { | ||
result = undefined; |
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.
You don't need to set it to undefined here. By default result would be undefined.
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 might be because of a lint complaint, but in this function is changed in the new implementation
/* Begin getProjectName */ | ||
async function getProjectName(context) { | ||
let projectName; | ||
if (context.exeInfo.inputParams.amplify && context.exeInfo.inputParams.amplify.projectName) { |
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 would break if I don't pass any params? Since inputParams
would be undefined or inputParams.amplify
might be undefined?
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.
it's guaranteed by the CLI at the beginning of the command execution.
context.exeInfo.metaData = { | ||
}; | ||
function normalizeProjectName(projectName) { | ||
if (!projectName) { |
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.
when would this branch get hit? I assume process.cwd() would always be defined?
/* Begin getEditor */ | ||
async function getEditor(context) { | ||
let editor; | ||
if (context.exeInfo.inputParams.amplify && context.exeInfo.inputParams.amplify.editor) { |
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.
Same as above-this would break the CLI if I don't pass any params? Since inputParams would be undefined or inputParams.amplify might be undefined?
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.
inputParams is guaranteed by the cli at the beginning of the execution.
context.exeInfo.projectConfig[constants.Label].config = | ||
frameworkConfigMapping[inputParams.framework]; | ||
} | ||
} else if (!context.exeInfo.inputParams.yes) { |
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.
Same as above. Shouldn't be using the yes flag to go into this branch.
@@ -4,7 +4,8 @@ | |||
"no-param-reassign": "off", | |||
"no-shadow": "off", | |||
"no-return-await" : "off", | |||
"no-continue": "off", | |||
"no-continue": "off", | |||
"no-plusplus": "off", |
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 shouldn't be off. Its inconsistent with other packages style. Style-wise it is recommended to use i +=1
rather than i++
.
\"region\":\"us-east-1\"\ | ||
}" | ||
AWSCLOUDFORMATION="{\ | ||
\"configLevel\":\"project\",\ |
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 param doesn't make a lot of sense to the user. Should be replaced or inferred form other flags/params.
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 can be removed, and by default the configuration is done on the project level. e.g. if the user doesn't know about it, it's not going to hurt him.
async function init(context) { | ||
context.projectConfigInfo = {}; | ||
normalizeInputParams(context); | ||
context.exeInfo.awsConfigInfo = { |
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.
Really hard to infer as to what the configLevel means.
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.
see above comments, the user can ignore it if he does not know about it, and it won't hurt him, by default it's on the project level,
const inputParams = context.exeInfo.inputParams[constants.Label]; | ||
Object.assign(awsConfigInfo, inputParams); | ||
} else if (awsConfigInfo.configLevel === 'project' && | ||
!context.exeInfo.inputParams.yes) { |
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.
Same as above. User doesn't need to explicitly mention yes flag.
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 needed, without the yes flag it's the normal init workflow. because the cli then needs to gather information from the user.
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 changes to 'packages/amplify-frontend-ios' is missing completly in this PR.
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.
As a general thing can we write an wrapper around inquirer which takes the if something is running in headless mode, the argument that was passed in the command line which would be the default value.
Something akin to this implementation
function inquireWhenNotHeadless(questions, commandlineParams, isHeadless) {
if (isHeadless) {
const result = {};
questions.forEach((question) => {
const { name, default as defaultValue } = question;
result[name] = commandLineParams[name] || defaultValue;
} )
return result;
}
return inquirer.prompt(questions);
}
} | ||
|
||
if (!frontend && context.exeInfo.inputParams.yes) { | ||
frontend = 'javascript'; |
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.
Shouldn't this be the default frontend?
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.
It is.
/* Begin getProjectName */ | ||
async function getProjectName(context) { | ||
let projectName; | ||
if (context.exeInfo.inputParams.amplify && context.exeInfo.inputParams.amplify.projectName) { |
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.
May be its better to use a util like loadash _.at
--amplify $AMPLIFY \ | ||
--javascript $JAVASCRIPT \ | ||
--aws $AWSCLOUDFORMATION \ | ||
--yes |
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.
Instead of --yes
can we change this to --headless
@@ -0,0 +1,38 @@ | |||
#!/bin/bash |
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.
Could we also add support for .
config params and normalize them to an object. For instance amplify config could be written as below
amplify configure project \
--amplify.projectName "$PROJECTNAME"
--amplify.defaultEditor "$EDITOR"
--amplify.prioiders awscloudformation
--amplify.frontend javascript
--javascript $JAVASCRIPT \
--awscloudformation $AWSCLOUDFORMATION
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 this might be a good idea since passing complex values on command line is messy.
@@ -0,0 +1,39 @@ | |||
#!/bin/bash |
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.
Can we use the default values if in headless mode if values are not passed. For instance, user should be able to skip if they are using create-react-app
REACTCONFIG="{\
\"SourceDir\":\"src\",\
\"DistributionDir\":\"build\",\
\"BuildCommand\":\"npm run-script build\",\
\"StartCommand\":\"npm run-script start\"\
}"
const inputParams = context.exeInfo.inputParams[constants.Label]; | ||
if (inputParams) { | ||
Object.assign(context.exeInfo.projectConfig[constants.Label], inputParams); | ||
} else if (!context.exeInfo.inputParams.yes) { |
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.
Conversely it should infer the default values when it is running in headless mode
|
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 work. Looks good :D
* init headless * add headless for javascript frontend * setup headless for android frontend * move publish ignore to a file inside hosting folder * move publish ignore out of amplifyrc file
* init headless * add headless for javascript frontend * setup headless for android frontend * move publish ignore to a file inside hosting folder * move publish ignore out of amplifyrc file
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
Use the following two bash shell scripts as examples to test the headless execution of the init and configure commands
packages/amplify-cli/tests/amplify-helpers/headless-init.sh
packages/amplify-cli/tests/amplify-helpers/headless-config.sh
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.