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

Improve nodejs example to show subdirectories sync #2024

Merged
merged 2 commits into from
May 6, 2019

Conversation

ilyeshammadi
Copy link
Contributor

I've updated the nodejs example to show the use of the undocumented folder/*** sync feature that syncs subdirectories. My changes consist of just placing the javascript files in multiple folders and to get the attention of the developer learning skaffold to pay attention when checking the skaffold.yaml file on the difference between the folder/** and folder/*** sync mechanisms.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@ilyeshammadi
Copy link
Contributor Author

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@codecov-io
Copy link

codecov-io commented Apr 25, 2019

Codecov Report

Merging #2024 into master will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2024      +/-   ##
==========================================
+ Coverage   55.96%   56.03%   +0.07%     
==========================================
  Files         173      175       +2     
  Lines        7566     7615      +49     
==========================================
+ Hits         4234     4267      +33     
- Misses       2928     2940      +12     
- Partials      404      408       +4
Impacted Files Coverage Δ
pkg/skaffold/schema/validation/validation.go 100% <0%> (ø) ⬆️
pkg/skaffold/schema/versions.go 74.35% <0%> (ø) ⬆️
pkg/skaffold/build/kaniko/types.go
pkg/skaffold/build/kaniko/sources/localdir.go
pkg/skaffold/build/kaniko/sources/gcs.go
pkg/skaffold/build/kaniko/run.go
pkg/skaffold/build/kaniko/sources/sources.go
pkg/skaffold/build/kaniko/kaniko.go
pkg/skaffold/build/kaniko/secret.go
pkg/skaffold/build/kaniko/logs.go
... and 11 more

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 9a68f2a...b4f4791. Read the comment docs.

Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

This LGTM!

Looks like there is documentation around this feature, and skaffold.dev should be updated after the next release.

Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

Whoops, one thing to fix -- could you make these changes in integration/examples/nodejs instead of examples/nodejs? The way we handle config changes requires this.

@ilyeshammadi
Copy link
Contributor Author

@priyawadhwa should be fixed now!

@priyawadhwa priyawadhwa added the kokoro:run runs the kokoro jobs on a PR label Apr 29, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 29, 2019
@tejal29 tejal29 added the kokoro:run runs the kokoro jobs on a PR label Apr 29, 2019
@tejal29
Copy link
Member

tejal29 commented Apr 29, 2019

@Ilyes-Hammadi , can you make the change in both files? so they are in both dir
examples are served as user samples and integration/examples are tested

@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 30, 2019
@ilyeshammadi
Copy link
Contributor Author

ilyeshammadi commented Apr 30, 2019

@tejal29 The changes are now in both folders.

@tejal29 tejal29 dismissed priyawadhwa’s stale review May 6, 2019 19:48

addressed comments

@tejal29 tejal29 merged commit ba63c30 into GoogleContainerTools:master May 6, 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.

6 participants