Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Feat: add traffic mesh proxy flag #1196

Merged
merged 13 commits into from
Sep 2, 2020
Merged

Conversation

erezrokah
Copy link
Contributor

@erezrokah erezrokah commented Sep 2, 2020

Fixes https://github.com/netlify/traffic-mesh/issues/806

This is a big PR since I did some refactoring to the dev command in the process since it was very hard to follow.
The commits are small though.

I'll add comments to clarify the changes.

This PR consists of 3 main changes:

  1. Extract common logic to download binary clients (this was used in the tunnel client and now shared with traffic mesh agent).
  2. Extract the different dev command responsibilities (starting functions server, framework server, Netlify dev proxy, tunnel client, etc) into separate functions/files.
  3. Traffic Mesh flag implementation which was quite small.

When enabling traffic mesh some test are still failing - looking into those.

@github-actions github-actions bot added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Sep 2, 2020
@erezrokah erezrokah mentioned this pull request Sep 2, 2020
@@ -0,0 +1,90 @@
const fs = require('fs-extra')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the logic here was extracted from https://github.com/netlify/cli/blob/93468a3fc43db5d637b30b9cbb21042da8626962/src/utils/live-tunnel.js

And made generic so we can re-use it.

})
return outdated
} catch (e) {
if (exists) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a small fix to use existing version if checking for updates failed.
That can happen when reaching GitHub API limit:
netlify/gh-release-fetch#3

}
})

const packages = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feels a bit strange to have this here and not in a dedicated test file (one for the tunnel client and one for the traffic mesh).
However I prefer it this way since we're actually testing the exec-fetcher

@@ -0,0 +1,15 @@
const path = require('path')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've started a new concept of libs instead of utils that we currently have.
It seems that most of our current utils are not used for sharing code, but just to extract functionality from the different commands.
However some commands/utils need to share the same code - so I started putting those parts under lib.

if (isWindows()) {
return true
}
const startLiveTunnel = async ({ siteId, netlifyApiToken, localPort, log }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

createTunnel and connectTunnel were called separately in the dev command.
I grouped them into startLiveTunnel

@@ -0,0 +1,346 @@
const path = require('path')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was mostly copied as is from the dev command

@@ -384,8 +29,11 @@ async function startDevServer(settings, log) {
},
})

server.start(function() {
log(`\n${NETLIFYDEVLOG} Server listening to`, settings.frameworkPort)
await new Promise(resolve => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some code to wait until the server is ready

}

async function startProxy(settings = {}, addonUrls, configPath, projectDir) {
const functionsServer = settings.functionsPort ? `http://localhost:${settings.functionsPort}` : null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original startProxy had some code to wait for the framework server port and functions server port:


if (functionsDir && settings.functionsPort) {

I removed it in favour of:

  1. Using waitPort in the function that starts the framework server: https://github.com/netlify/cli/pull/1196/files#diff-8a2f65e3aede3ea9696e546766f50f03R82
  2. When starting the functions server - wait on a promise until the server is ready: https://github.com/netlify/cli/pull/1196/files#diff-cf2bd94d7688f0711bbd931e6c5e0397R373

proxy.ws(req, socket, head)
})

return new Promise(resolve => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also added this code to wait for the dev server to be ready


return new Promise(resolve => {
server.listen({ port: settings.port }, () => {
resolve(`http://localhost:${settings.port}`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original startProxy returned an object with url and port as properties:

return { url: `http://localhost:${settings.port}`, port: settings.port }

but only url was used.
Simplified the function to return just the url as a string

@@ -318,4 +310,77 @@ async function serveFunctions(dir, siteInfo = {}) {
return app
}

module.exports = { serveFunctions }
const getBuildFunction = ({ functionBuilder, log }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this chunk of code from the dev command

const functionsServer = await serveFunctions(settings.functions, siteInfo)

await new Promise(resolve => {
functionsServer.listen(settings.functionsPort, err => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this to wait until the server is ready

@@ -0,0 +1,70 @@
const path = require('path')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is very similar to the tunnel client code

)

try {
const open = await waitPort({ port, output: 'silent', timeout: 30 * 1000 })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait for the agent to be ready (the traffic mesh agent writes to a log file and not stdout) so waiting on the port works well.

path: 'index.html',
content: '<h1>⊂◉‿◉つ</h1>',
})
const testMatrix = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes in this file is for support a test matrix with different args (and creating unique test names)

@erezrokah erezrokah requested a review from ehmicky September 2, 2020 13:40
Copy link
Contributor

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

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

🚀

@erezrokah
Copy link
Contributor Author

@ehmicky wow that was fast. Going to disable the new failing test (they are failing to due GitHub API limit reached on CI).

@erezrokah erezrokah merged commit 894ed44 into master Sep 2, 2020
@erezrokah erezrokah deleted the feat/add_traffic_mesh_proxy branch September 2, 2020 15:22
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants