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

Fixed Dashboard v0.0.1 w/ Polymer3 + Webpack #2576

Merged
merged 11 commits into from
Mar 4, 2019
20 changes: 15 additions & 5 deletions components/centraldashboard/make-win.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ $CHANGED_FILES = git diff-files --relative=components/centraldashboard
$GIT_VERSION = git describe --always
if ($CHANGED_FILES) {
$GIT_VERSION = "$GIT_VERSION-dirty-$(git diff | sha256sum | cut -c -6)"
Write-AP "!You are building from a dirty branch, it's recommended that you commit and push all changes before building!"
}

$Date = (Get-Date).ToString("vyyyyMMdd")
Expand All @@ -40,21 +41,30 @@ switch($step) {
--build-arg kubeflowversion=$(git describe --abbrev=0 --tags) `
--label=git-verions=$GIT_VERSION
docker tag "${IMG}:$TAG" "${IMG}:latest"
Write-AP "+Built '${IMG}:latest'"
Write-AP "+Built '${IMG}:$TAG'"
break
if ($?) {
Write-AP "x+","nx_Built ","n+${IMG}:latest"
Write-AP "x+","nx_Built ","n+${IMG}:$TAG"
} else {
Write-AP "-Failed to build [${IMG}:$TAG]"
}
}
"push" {
# Build but don't attach the latest tag. This allows manual testing/inspection of the image
# first.
gcloud docker -- push "${IMG}:$TAG"
Write-AP "+Pushed $IMG with :$TAG tags"
Write-AP $(if ($?)
{"x+","nx_Pushed ","nx+$IMG","nx_ with tag ","nx+:$TAG","n_ tags"}else
{"-Failed to push ${IMG}:$TAG"}
)
}
"push-latest" {
# Build but don't attach the latest tag. This allows manual testing/inspection of the image
# first.
gcloud container images add-tag --quiet "${IMG}:$TAG" "${IMG}:latest" --verbosity=info
Write-AP "+Created ${IMG}:latest tags"
Write-AP $(if ($?)
{"x+","nx_Pushed ","nx+${IMG}:latest","n_ tags"}else
{"-Failed to push ${IMG}:latest tags"}
)
}
}

69 changes: 39 additions & 30 deletions components/centraldashboard/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion components/centraldashboard/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"version": "0.0.1",
"description": "Kubeflow Central Dashboard",
"scripts": {
"build": "NODE_ENV=production npx webpack && npm run tslint && npm run build-ts",
"build": "cross-env NODE_ENV=production npx webpack && npm run tslint && npm run build-ts",
"build-ts": "tsc",
"build-webpack": "npx webpack",
"debug": "npm run build && npm run watch-debug",
Expand All @@ -28,6 +28,7 @@
},
"homepage": "https://github.com/kubeflow/kubeflow#readme",
"dependencies": {
"@babel/polyfill": "^7.2.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is needed. I know there is the warning in the webpack build process but I think the fact that we are providing the dependencies through the webcomponents-loader.js makes it a moot point. At best I think it would be a devDependency though and not in our primary deps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind. I think I overestimated the scope of the webcomponents polyfills as also including standard polyfills for ES6. I do think we still need to import @babel/polyfill in our entry point.

I've added that to my PR #2579. Feel free to merge the applicable changes from that into this change, or if you want to approve, we can merge that into master first and then your change will only be to fix the Windows specific stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, let's merge yours, since it addresses a bug, and then this can be a feature merge.

"@polymer/app-layout": "^3.0.0",
"@polymer/app-route": "^3.0.0",
"@polymer/iron-collapse": "^3.0.1",
Expand Down Expand Up @@ -59,6 +60,7 @@
"clean-webpack-plugin": "^1.0.1",
"concurrently": "^4.1.0",
"copy-webpack-plugin": "^5.0.0",
"cross-env": "^5.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I also noticed that the terser-webpack-plugin got added to deps instead of devDeps. Can you move that in this commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

"css-loader": "^2.1.0",
"exports-loader": "^0.7.0",
"file-loader": "^3.0.1",
Expand Down