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 for #5210 #5790

Closed
wants to merge 1 commit into from
Closed

Fix for #5210 #5790

wants to merge 1 commit into from

Conversation

casret
Copy link

@casret casret commented May 5, 2021

Fixes: #5210
Description

Remove these from the unrecoverable errors list. Containers are
ephemeral in k8s, so errors in them may be recoverable at a system
level. E.g. when they are waiting for another resource to stablelize.

User facing changes (remove if N/A)

The deploy status check will wait for the deployment to stabilize even if the container errors out. This restores the 1.12 behaviour.

Remove these from the unrecoverable errors list.  Containers are
ephemeral in k8s, so errors in them may be recoverable at a system
level.  E.g. when they are waiting for another resource to stablelize.
@casret casret requested a review from a team as a code owner May 5, 2021 18:25
@casret casret requested a review from MarlonGamez May 5, 2021 18:25
@google-cla
Copy link

google-cla bot commented May 5, 2021

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 with @googlebot 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.

@casret
Copy link
Author

casret commented May 5, 2021

@googlebot I signed it!

@google-cla
Copy link

google-cla bot commented May 5, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@casret
Copy link
Author

casret commented May 5, 2021

Too many hoops to associate my email address. I put these whopping 2 lines of code in the public domain.

@codecov
Copy link

codecov bot commented May 5, 2021

Codecov Report

Merging #5790 (56192e1) into master (d14f34d) will increase coverage by 0.00%.
The diff coverage is 42.42%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #5790    +/-   ##
========================================
  Coverage   70.78%   70.79%            
========================================
  Files         432      436     +4     
  Lines       16242    16356   +114     
========================================
+ Hits        11497    11579    +82     
- Misses       3901     3923    +22     
- Partials      844      854    +10     
Impacted Files Coverage Δ
cmd/skaffold/skaffold.go 0.00% <0.00%> (ø)
pkg/skaffold/build/jib/init.go 83.05% <0.00%> (-4.45%) ⬇️
pkg/skaffold/build/jib/jib.go 69.46% <0.00%> (-1.08%) ⬇️
pkg/skaffold/deploy/resource/deployment.go 89.65% <ø> (ø)
pkg/skaffold/util/tar.go 50.66% <0.00%> (ø)
cmd/skaffold/app/cmd/cmd.go 63.80% <50.00%> (ø)
pkg/skaffold/color/formatter.go 96.00% <50.00%> (-4.00%) ⬇️
pkg/skaffold/build/jib/jvm.go 55.55% <55.55%> (ø)
pkg/skaffold/util/term.go 52.94% <60.00%> (+10.08%) ⬆️
pkg/skaffold/docker/image.go 78.34% <0.00%> (-1.39%) ⬇️
... 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 a9a18d8...56192e1. Read the comment docs.

@tejal29
Copy link
Contributor

tejal29 commented May 5, 2021

This behavior was added intentionally to quit status check on exited containers especially for GCP cloud run integration.
With that said, I do understand your use case.

Maybe we could achieve this via statusCheck config.

statusCheck:
   waitOnContainerTermninated: true

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

This behavior was added for Cloud run to make sure status check terminates quickly.

This needs further exploration on

  1. When to terminate quickly Vs when to wait.
  2. Shd it be user configurable?

@tejal29
Copy link
Contributor

tejal29 commented May 10, 2021

Too many hoops to associate my email address. I put these whopping 2 lines of code in the public domain.

Sorry @casret, but we do need you to sign the CLA. Let us know if you don't plan to work on this anymore.

@casret
Copy link
Author

casret commented May 10, 2021 via email

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@tejal29
Copy link
Contributor

tejal29 commented May 12, 2021

I would love to, and I did, but it won't take my email address. Not sure where to go from here.

On Tue, May 11, 2021 at 7:25 AM Tejal Desai @.***> wrote: Too many hoops to associate my email address. I put these whopping 2 lines of code in the public domain. Sorry @casret https://github.com/casret, but we do need you to sign the CLA. Let us know if you don't plan to work on this anymore. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5790 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACLBFO6YUAICPZVOVXHNDTNBTPXANCNFSM44FQWPKQ .

Just wanted to make sure, you are signing the CLA for email address associated with your github profile.
If you hv another email address (e.g. gmail), you can associate with your github profile as secondary email and sign the CLA using the secondary email.

Please let me know if that works.

@casret
Copy link
Author

casret commented May 13, 2021

@googlebot I fixed it

@google-cla
Copy link

google-cla bot commented May 13, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: yes and removed cla: no labels May 13, 2021
@casret
Copy link
Author

casret commented May 13, 2021

@tejal29 Thanks for guiding me through the process. Wasn't very clear from the instructions where I was to add the alternate email. I thought It was to my google account. Now that the red tape is cleared, I'll try a version with the config.

@tejal29
Copy link
Contributor

tejal29 commented May 13, 2021

@casret Please see your development guide section to make config changes in DEVELOPMENT.md

@tejal29
Copy link
Contributor

tejal29 commented May 25, 2021

@casret ping!

@tejal29
Copy link
Contributor

tejal29 commented Jun 1, 2021

@casret Cleaning up older PRs. Closing this as the solution mentioned here is drastically different than the current approach. Please open a new PR with the discussed solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skaffold fails when a container's status code is 1
3 participants