-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
@ #1379 | Bug fix for Helm imageStrategy file sync #2111
@ #1379 | Bug fix for Helm imageStrategy file sync #2111
Conversation
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. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Codecov Report
@@ Coverage Diff @@
## master #2111 +/- ##
==========================================
- Coverage 60.59% 56.37% -4.22%
==========================================
Files 185 180 -5
Lines 7806 7794 -12
==========================================
- Hits 4730 4394 -336
- Misses 2687 2982 +295
- Partials 389 418 +29
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening getting started on this!
- Do you mind adding a test to cover this?
- I would love to see the a bit more evidence that this is a fix for Skaffold failing to sync with helm #1379 - do you mind adding more step by step detail how this is connected to sync error / fqn vs not fqn?
Thank you!
Hi @hoatle! Do you need some help to finish this PR? |
5896a0d
to
7552cff
Compare
7552cff
to
5a77a65
Compare
Hi @balopat @dgageot,
I hope that the example will help to clarify the problem. |
👍 would love to have this in a release |
Hi @hoatle since you added a sample to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment
@hoatle any chance you could update this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the extractTag
code change and move the examples/helm-deployment-hot-reloadto
integration/examples/helm-deployment-hot-reload`
closing due to inactivity. Also the code change was fixed in another PR |
fix for #1379