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

Add ProgressDeadlineSeconds #54

Merged
merged 2 commits into from Apr 23, 2019
Merged

Add ProgressDeadlineSeconds #54

merged 2 commits into from Apr 23, 2019

Conversation

dthomson25
Copy link
Member

Additionally, I simplified the testing framework to allow developers to test specific fields within the objects sent to the Kubernetes API instead of the entire object.

@codecov-io
Copy link

codecov-io commented Apr 19, 2019

Codecov Report

Merging #54 into master will increase coverage by 0.96%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
+ Coverage   76.14%   77.11%   +0.96%     
==========================================
  Files          14       14              
  Lines        1417     1595     +178     
==========================================
+ Hits         1079     1230     +151     
- Misses        259      278      +19     
- Partials       79       87       +8
Impacted Files Coverage Δ
controller/service.go 86.45% <100%> (+1.4%) ⬆️
utils/defaults/defaults.go 100% <100%> (ø) ⬆️
controller/bluegreen.go 74.8% <100%> (+0.17%) ⬆️
controller/canary.go 81.31% <100%> (+0.1%) ⬆️
controller/controller.go 50.27% <66.66%> (+4.2%) ⬆️
controller/sync.go 72% <69.36%> (+1.62%) ⬆️
utils/conditions/conditions.go 89.11% <86.88%> (-1.8%) ⬇️

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 302568e...370dc57. Read the comment docs.

Copy link
Contributor

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

Added few comments. Can you use Patch instead of Update? Otherwise, we might get concurrent update errors.

c.recorder.Eventf(r, corev1.EventTypeWarning, conditions.ServiceNotFoundReason, msg)
cond := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionFalse, conditions.ServiceNotFoundReason, msg)
conditions.SetRolloutCondition(&r.Status, *cond)
c.rolloutsclientset.ArgoprojV1alpha1().Rollouts(r.Namespace).Update(r)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use Patch to set conditions.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

c.recorder.Eventf(r, corev1.EventTypeWarning, conditions.ServiceNotFoundReason, msg)
cond := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionFalse, conditions.ServiceNotFoundReason, msg)
conditions.SetRolloutCondition(&r.Status, *cond)
c.rolloutsclientset.ArgoprojV1alpha1().Rollouts(r.Namespace).Update(r)
Copy link
Contributor

Choose a reason for hiding this comment

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

Return Update/Patch error

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@@ -130,7 +134,11 @@ func (c *Controller) getPreviewAndActiveServices(r *v1alpha1.Rollout) (*corev1.S
activeSvc, err = c.kubeclientset.CoreV1().Services(r.Namespace).Get(r.Spec.Strategy.BlueGreenStrategy.ActiveService, metav1.GetOptions{})
if err != nil {
if errors.IsNotFound(err) {
logCtx.Warnf("Service %v does not exist", r.Spec.Strategy.BlueGreenStrategy.PreviewService)
msg := fmt.Sprintf(conditions.ServiceNotFoundMessage, "active", r.Spec.Strategy.BlueGreenStrategy.ActiveService)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as for line 126

Copy link
Member Author

Choose a reason for hiding this comment

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

👍


// Reasons and Messages for rollout conditions

// Progressing:
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

Nice!

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

Successfully merging this pull request may close these issues.

None yet

3 participants