-
Notifications
You must be signed in to change notification settings - Fork 685
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
Added feature to pick from multiple sample backends. #2853
Changes from 37 commits
36eb075
3e3e1c6
05a9edf
f2b1074
5e21ed0
237dd12
487bc7b
6c6786a
6dfc72b
1ab9cce
d8e25ef
0a80953
1ed5283
240c0a2
fac9c0b
c57b13d
df3dc18
d0516fa
8b5cf97
423f002
dc3da4d
b3110ef
71e35eb
7323d16
bd02431
315b8cd
e56371e
dd19131
4856d57
ee19c77
acebbce
774fa80
f04d371
0445d2e
1533416
fed64cc
deccd28
8aaf222
ebb42e0
dbb8cf9
34b1a30
abd0613
b55b6df
a4d2154
98abb1b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,36 @@ | ||
const { basename, resolve } = require('path'); | ||
const os = require('os'); | ||
const fetch = require('node-fetch'); | ||
const changeCase = require('change-case'); | ||
const inquirer = require('inquirer'); | ||
const execa = require('execa'); | ||
const chalk = require('chalk'); | ||
const gitUserInfo = require('git-user-info'); | ||
const isInvalidPath = require('is-invalid-path'); | ||
const isValidNpmName = require('is-valid-npm-name'); | ||
const { uniqBy } = require('lodash'); | ||
|
||
const pkg = require('../package.json'); | ||
const { | ||
sampleBackends | ||
sampleBackends: defaultSampleBackends | ||
} = require('@magento/pwa-buildpack/lib/cli/create-project'); | ||
|
||
const removeDuplicateBackends = backendEnvironments => | ||
uniqBy(backendEnvironments, 'url'); | ||
|
||
const fetchSampleBackends = async () => { | ||
try { | ||
const res = await fetch( | ||
'https://fvp0esmt8f.execute-api.us-east-1.amazonaws.com/default/getSampleBackends' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am looking at AWS docs to use a DNS name instead of this raw path. It will be changed in the future (before this PR is merged). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a lambda? Does it read configuration somewhere? I'm a bit surprised we don't just maintain a list in the extension. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The plan is to pull these backend URLs dynamically so we don't need to publish every single time a backend changes, which we will have to if these are part of the package. The URL above is an AWS Gateway URL. The plan is to use a domain-specific URL so in the future if we move between cloud providers (Azure, Ethos, etc), we can keep using the same URL but the implementation in the back can change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Something will have to change somewhere whether that's a file in s3, a lambda script, or a local file in a pwa studio extension/package (like create-pwa). And as we saw in the demo, there is already a file for hosting these, but within buildpack. I personally prefer keeping this file locally within If the team want to keep this solution as is, I'd really like to see some documentation with steps to update this file. I also think we should understand the costs of this approach. How much per request does this lambda/endpoint cost us? Edit: To be clear, I appreciate the approach as an example for how we could interact with a micro-service. It's a neat example for sure. I just think it's overkill for us. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The points you have mentioned are perfectly reasonable. I did have a thought of making it a package to avoid the cost of hosting and also avoid potential DoS security issues. But the biggest problem with publishing on NPM is that developers don't generally run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Two questions:
I think I see that it uses it on initial scaffold but then only later if the up-check fails. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Along with bootstrapping, It will be used every time someone adds the
It is used during the build stage if the extension is installed. It will be used to fetch the sample backends, if the current backend is inactive. We won't be calling the service if the backend mentioned in the ENV file is active. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if this URL goes down? Will the yarn install / build hang for X seconds? Also if we're maintaining this micro server we need to setup monitoring to ensure we can be aware of uptime issues. We should also consider any security implications of having this service running. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice question @davemacaulay. I have used a wrong URL to simulate the service failure and you can see here, if the service is down, the script will use the internal URLs shipped as part of the You are right about monitoring. I'll create a ticket to investigate the monitoring capabilities of our team/AWS. Coming to security, we are not giving the public access to the Lamda function or the S3 object. It is an AWS Gateway Service that we are using. Also, the Lamda function that we are using only has read access to that particular object. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @revanth0212 It would nice if we can move the lambda setup to aws prod account and check if any Audits fail. Rest all looks good. |
||
); | ||
const { sampleBackends } = await res.json(); | ||
|
||
return sampleBackends.environments; | ||
} catch { | ||
return []; | ||
} | ||
}; | ||
|
||
module.exports = async () => { | ||
console.log(chalk.greenBright(`${pkg.name} v${pkg.version}`)); | ||
console.log( | ||
|
@@ -20,6 +39,16 @@ module.exports = async () => { | |
const userAgent = process.env.npm_config_user_agent || ''; | ||
const isYarn = userAgent.includes('yarn'); | ||
|
||
const sampleBackendEnvironments = await fetchSampleBackends(); | ||
const filteredBackendEnvironments = removeDuplicateBackends([ | ||
...sampleBackendEnvironments, | ||
...defaultSampleBackends.environments | ||
]); | ||
const sampleBackends = { | ||
...defaultSampleBackends, | ||
environments: filteredBackendEnvironments | ||
}; | ||
|
||
const questions = [ | ||
{ | ||
name: 'directory', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`should call onFail if backend is inactive 1`] = ` | ||
"https://www.magento-backend-2.3.4.com/ is inactive. Please consider using one of these other backends: | ||
|
||
[{\\"name\\":\\"2.3.3-venia-cloud\\",\\"description\\":\\"Magento 2.3.3 with Venia sample data installed\\",\\"url\\":\\"https://master-7rqtwti-mfwmkrjfqvbjk.us-4.magentosite.cloud/\\"}] | ||
" | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
jest.mock('node-fetch'); | ||
const fetch = require('node-fetch'); | ||
|
||
const { validateSampleBackend } = require('../intercept'); | ||
|
||
const env = { | ||
MAGENTO_BACKEND_URL: 'https://www.magento-backend-2.3.4.com/' | ||
}; | ||
const onFail = jest.fn().mockName('onFail'); | ||
const debug = jest.fn().mockName('debug'); | ||
|
||
const args = { env, onFail, debug }; | ||
|
||
test('should not call onFail if backend is active', async () => { | ||
fetch.mockResolvedValueOnce({ ok: true }); | ||
|
||
await validateSampleBackend(args); | ||
|
||
expect(onFail).not.toHaveBeenCalled(); | ||
}); | ||
|
||
test('should call onFail if backend is inactive', async () => { | ||
fetch.mockResolvedValueOnce({ ok: false }).mockResolvedValueOnce({ | ||
json: jest.fn().mockResolvedValue({ | ||
sampleBackends: { | ||
environments: [ | ||
{ | ||
name: '2.3.3-venia-cloud', | ||
description: | ||
'Magento 2.3.3 with Venia sample data installed', | ||
url: | ||
'https://master-7rqtwti-mfwmkrjfqvbjk.us-4.magentosite.cloud/' | ||
}, | ||
{ | ||
name: '2.3.4-venia-cloud', | ||
description: | ||
'Magento 2.3.4 with Venia sample data installed', | ||
url: 'https://www.magento-backend-2.3.4.com/' | ||
} | ||
] | ||
} | ||
}) | ||
}); | ||
|
||
await validateSampleBackend(args); | ||
|
||
expect(onFail).toHaveBeenCalled(); | ||
expect(onFail.mock.calls[0][0]).toMatchSnapshot(); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
const fetch = require('node-fetch'); | ||
|
||
const isBackendActive = async (env, debug) => { | ||
try { | ||
const magentoBackend = env.MAGENTO_BACKEND_URL; | ||
const res = await fetch(magentoBackend); | ||
|
||
return res.ok; | ||
} catch (err) { | ||
debug(err); | ||
|
||
return false; | ||
} | ||
}; | ||
|
||
const fetchBackends = async debug => { | ||
try { | ||
const res = await fetch( | ||
'https://fvp0esmt8f.execute-api.us-east-1.amazonaws.com/default/getSampleBackends' | ||
); | ||
const { sampleBackends } = await res.json(); | ||
|
||
return sampleBackends.environments; | ||
} catch (err) { | ||
debug(err); | ||
|
||
return []; | ||
} | ||
}; | ||
|
||
/** | ||
* Validation function to check if the backend being used is one of the sample backends provided | ||
* by PWA Studio. If yes, the function validates if the backend is active. If not, it reports an | ||
* error by calling the onFail function. In the error being reported, it sends the other sample | ||
* backends that the developers can use. | ||
* | ||
* @param {Object} config.env - The ENV provided to the app, usually avaialable through process.ENV | ||
* @param {Function} config.onFail - callback function to call on validation fail | ||
* @param {Function} config.debug - function to log debug messages in console in debug mode | ||
* | ||
* To watch the debug messages, run the command with DEBUG=*runEnvValidators* | ||
*/ | ||
const validateSampleBackend = async config => { | ||
const { env, onFail, debug } = config; | ||
|
||
const backendIsActive = await isBackendActive(env, debug); | ||
|
||
if (!backendIsActive) { | ||
debug(`${env.MAGENTO_BACKEND_URL} is inactive`); | ||
|
||
debug('Fetching other backends'); | ||
|
||
const sampleBackends = await fetchBackends(debug); | ||
const otherBackends = sampleBackends.filter( | ||
({ url }) => url !== env.MAGENTO_BACKEND_URL | ||
); | ||
|
||
debug( | ||
'PWA Studio supports the following backends', | ||
sampleBackends.join('\n') | ||
); | ||
|
||
debug('Reporting backend URL validation failure'); | ||
onFail( | ||
`${ | ||
env.MAGENTO_BACKEND_URL | ||
} is inactive. Please consider using one of these other backends: \n\n ${JSON.stringify( | ||
otherBackends | ||
)} \n` | ||
); | ||
} else { | ||
debug(`${env.MAGENTO_BACKEND_URL} is active`); | ||
} | ||
}; | ||
|
||
module.exports = targets => { | ||
targets | ||
.of('@magento/pwa-buildpack') | ||
.validateEnv.tapPromise(validateSampleBackend); | ||
}; | ||
|
||
module.exports.validateSampleBackend = validateSampleBackend; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
{ | ||
"name": "@magento/venia-sample-backends", | ||
"version": "0.0.1", | ||
"publishConfig": { | ||
"access": "public" | ||
}, | ||
"description": "Provides demo backends and backend validation utils for PWA Studio.", | ||
"main": "./intercept.js", | ||
"scripts": { | ||
"clean": " ", | ||
"test": "jest" | ||
}, | ||
"repository": "github:magento/pwa-studio", | ||
"license": "(OSL-3.0 OR AFL-3.0)", | ||
"peerDependencies": { | ||
"@magento/pwa-buildpack": "~7.0.0", | ||
"node-fetch": "~2.3.0" | ||
}, | ||
"pwa-studio": { | ||
"targets": { | ||
"intercept": "./intercept" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,4 +20,4 @@ | |
"intercept": "./i18n-intercept" | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -153,7 +153,26 @@ module.exports = targets => { | |
* @member {tapable.AsyncSeriesHook} | ||
* @param {transformUpwardIntercept} interceptor | ||
*/ | ||
transformUpward: new targets.types.AsyncSeries(['definitions']) | ||
transformUpward: new targets.types.AsyncSeries(['definitions']), | ||
|
||
/** | ||
* Collect all ENV validation functions that will run against the | ||
* project's ENV. The functions can be async and they will run in | ||
* parallel. If a validation function wants to stop the whole process | ||
* for instance in case of a serious security issue, it can do so | ||
* by throwing an error. If it wants to report an error, it can do so | ||
* by using the onFail callback provided as an argument. A validation | ||
* function can submit multiple errors by calling the onFail function | ||
* multiple times. All the errors will be queued into an array and | ||
* displayed on the console at the end of the process. | ||
* | ||
* @example | ||
* targets.of('@magento/pwa-buildpack').validateEnv.tapPromise(validateBackendUrl); | ||
* | ||
* @member {tapable.AsyncParallelHook} | ||
* @param {envValidationInterceptor} validator | ||
*/ | ||
validateEnv: new targets.types.AsyncParallel(['validator']) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excellently documented! Thank you very very much. |
||
}; | ||
|
||
/** | ||
|
@@ -282,3 +301,29 @@ module.exports = targets => { | |
* @param {object} definition - Parsed UPWARD definition object. | ||
* @returns {Promise} | ||
*/ | ||
|
||
/** Type definitions related to: validateEnv */ | ||
|
||
/** | ||
* Intercept function signature for the validateEnv target. | ||
* | ||
* Interceptors of the `validateEnv` target receive a config object. | ||
* The config object contains the project env, an onFail callback and | ||
* the debug function to be used in case of the debug mode to log more | ||
* inforamtion to the console. | ||
* | ||
* This Target can be used asynchronously in the parallel mode. If a | ||
* validator needs to stop the process immediately, it can throw an error. | ||
* If it needs to report an error but not stop the whole process, it can do | ||
* so by calling the onFail function with the error message it wants to report. | ||
* It can call the onFail multiple times if it wants to report multiple errors. | ||
* | ||
* All the errors will be queued and printed into the console at the end of the | ||
* validation process and the build process will be stopeed. | ||
* | ||
* @callback envValidationInterceptor | ||
* @param {Object} config.env - Project ENV | ||
* @param {Function} config.onFail - On fail callback | ||
* @param {Function} config.debug - Debug function to be used for additional reporting in debug mode | ||
* @returns {Boolean} | ||
*/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`should throw error if there are validation errors reported by interceptors 1`] = ` | ||
"Environment has 2 validation errors: | ||
(1) Danger, | ||
(2) Another error" | ||
`; |
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 was going to bring up per method imports, but it looks like lodash is getting rid of them in v5. My instinct here is that this is a heavy dependency for the utility, but I'm not sure if this same rule applies to creators. I'm not even sure a direct import would help. I'm leaning towards per method for now, but not required if you think this is reasonable as-is.
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 am a fan of the method imports myself. But I realized we import the whole lodash library as part of buildpack anyway. Hence I tried to add it as a peer dependency. Also, Lodash one of the libraries which are really good for tree shaking. Webpack should be able to remove all unnecessary code.
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 can stay. It's not an ES6 import, it's a destructure of a full import of Lodash. But that's okay!
Code in this package won't be built by buildpack; it'll run in Node. Many of the dependent packages are already importing all of lodash (
yarn why lodash
sums it up), so this import doesn't make a difference.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.
Works for me 👍 @revanth0212
lodash
is currently in thedependencies
list, did you intend to put it underpeerDependencies
? As long as it's not grabbing two different versions it shouldn't matter, just wanted to point it out.