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

Fix and improve sync samples #2131

Merged
merged 12 commits into from
May 17, 2019

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented May 16, 2019

  • Fixed the sync configuration on hot-reload sample
  • Fix all samples so that they actually work
  • Convert pods into deployments. Now that port forwarding is not activated, deployments need to be exposed with a service
  • Simplify the skaffold.yaml files with regards to deploy config.
  • Improve the Dockerfiles (caching, smaller and more recent base images...)

@codecov-io
Copy link

codecov-io commented May 16, 2019

Codecov Report

Merging #2131 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2131   +/-   ##
=======================================
  Coverage   56.33%   56.33%           
=======================================
  Files         180      180           
  Lines        7840     7840           
=======================================
  Hits         4417     4417           
  Misses       3002     3002           
  Partials      421      421

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 643b8a7...56ed26c. Read the comment docs.

@dgageot dgageot force-pushed the fix-sync-samples branch 4 times, most recently from 41e1926 to ed693aa Compare May 17, 2019 09:30
@dgageot
Copy link
Contributor Author

dgageot commented May 17, 2019

@corneliusweig Could you take a look at those fixes? Thanks!

@@ -5,9 +5,8 @@ build:
- image: gcr.io/k8s-skaffold/react-reload
context: app
sync:
'src/components/*': src/components/
Copy link
Contributor

Choose a reason for hiding this comment

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

oops.. this should not have happened (@.myself). The reason why it could is that react-reload has no test-case in run_test.go.


COPY package* ./
RUN npm install
COPY src .
Copy link
Contributor

@corneliusweig corneliusweig May 17, 2019

Choose a reason for hiding this comment

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

Suggested change
COPY src .
COPY src src/

Otherwise docker flattens the directory at the destination. And in the package.json the main file is src/index.js. I think it only works by accident, because node looks for index.js in the root folder if the entrypoint is not found.

The other examples are fine, because they use COPY . .

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 think the idea was to flatten the directory. in the skaffold.yaml, sync is configured to strip the src/ prefix.
I'll fix the package.json though.

Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

Thanks for this examples face-lift. Looks very nice overall. There's one nit and the following bigger issue:

All these examples are not run during integration tests (run_test.go):

examples/compose
examples/helm-deployment-dependencies
examples/hot-reload
examples/jib
examples/jib-multimodule
examples/kustomize
examples/react-reload

At least for react-reload I can say that I simply didn't know that a test-case needed to be added. Should we do that? Could also be another PR, because this one is already quite large.

dgageot added 11 commits May 17, 2019 14:26
The sample currently doesn’t work

Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
@dgageot
Copy link
Contributor Author

dgageot commented May 17, 2019

Good point. I'll push a PR to test more samples (not sure it's easy to test them all) after this PR is merged.

Signed-off-by: David Gageot <[email protected]>
@dgageot dgageot force-pushed the fix-sync-samples branch from ed693aa to 56ed26c Compare May 17, 2019 12:34
@corneliusweig
Copy link
Contributor

@dgageot LGTM

Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

nice, thanks @dgageot

@nkubala nkubala merged commit da5f201 into GoogleContainerTools:master May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants