Skip to content
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

[ZEPPELIN-2190] Support custom web development port #2113

Closed
wants to merge 3 commits into from

Conversation

soralee
Copy link
Contributor

@soralee soralee commented Mar 9, 2017

What is this PR for?

This PR is for support flexible port for custom web application development and update the structure of README.md document for the support variable.

What type of PR is it?

[ Improvement | Documentation (README.md) ]

What is the Jira issue?

How should this be tested?

  1. run build
    • mvn clean package -DskipTests -pl 'zeppelin-web'
  2. run dev mode with WEB_PORT variable port under zeppelin-web folder
    • WEB_PORT=9999 yarn run dev
  3. connect localhost:9999 on web browser

Screenshots (if appropriate)

[after]
image

image

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? Yes, README.md of zeppelin-web

@1ambda
Copy link
Member

1ambda commented Mar 9, 2017

I will test and give you feedback soon.

pattern: /WEB_PORT/ig,
replacement: function (match, p1, offset, string) {
return webPort;
}
Copy link
Member

Choose a reason for hiding this comment

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

MINOR: invalid indentation here.

config.plugins.push(new InsertLiveReloadPlugin())
config.plugins.push(
new InsertLiveReloadPlugin(),
// string replace plugin instance
Copy link
Member

@1ambda 1ambda Mar 9, 2017

Choose a reason for hiding this comment

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

MINOR: i think we can remove this comment line.

LINE 285

@@ -24,7 +24,7 @@ function baseUrlSrv() {
}
}
//Exception for when running locally via grunt
if (port === 9000) {
if (port === process.env.WEB_PORT) {
Copy link
Member

Choose a reason for hiding this comment

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

We can remove if statement by modifying this like

var serverPort = process.env.SERVER_PORT || 8080;
var webPort = process.env.WEB_PORT || 9000;

Copy link
Member

Choose a reason for hiding this comment

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

Oops. I wrote comments in different file. i meant webpack.config.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for my late response and I clearly understood your mentions!
Thanks @1ambda!

@soralee
Copy link
Contributor Author

soralee commented Mar 10, 2017

@1ambda Thanks for kind review! Let me update this soon!

@Leemoonsoo
Copy link
Member

ping @soralee

@soralee
Copy link
Contributor Author

soralee commented Mar 24, 2017

@1ambda Hi, I just updated as your comments.
Could you check this again? Thanks!

@1ambda
Copy link
Member

1ambda commented Mar 28, 2017

LGTM

@Leemoonsoo
Copy link
Member

LGTM and merge to master if no further discussions.

@asfgit asfgit closed this in 7b345a9 Mar 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants