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.

Sign up for GitHub

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

Jump to bottom

Reconcile Ingress when label was different from desired #9719

Merged
merged 3 commits into from
Oct 7, 2020
Merged

Reconcile Ingress when label was different from desired #9719

merged 3 commits into from
Oct 7, 2020

Conversation

nak3
Copy link
Contributor

@nak3 nak3 commented Oct 7, 2020

Proposed Changes

As per subject, this patch changes to reconcile Ingress when
it was different from desired.

Release Note

Ingress is reconciled when label was different from desired.

/cc @markusthoemmes @mattmoor @tcnghia

@nak3
Reconcile Ingress when label was different from desired
81bdc63 
As per subject, this patch changes to reconcile Ingress when
it was different from desired.
@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 7, 2020
@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/API API objects and controllers area/networking labels Oct 7, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nak3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 7, 2020
@nak3 nak3 mentioned this pull request Oct 7, 2020
Closed
markusthoemmes
markusthoemmes reviewed Oct 7, 2020
View reviewed changes
pkg/reconciler/route/reconcile_resources.go Outdated

func ingressSemanticEquals(ctx context.Context, desiredIngress, ingress *netv1alpha1.Ingress) (bool, error) {
logger := logging.FromContext(ctx)
specDiff, err := kmp.SafeDiff(desiredIngress.Spec, ingress.Spec)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we're adding this vs. just adding the labels DeepEqual + copying the labels over?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @markusthoemmes @julz
Yeah, I did this because of trying to align with other codes like

serving/pkg/reconciler/service/service.go

Line 195 in 4bfda35

func configSemanticEquals(ctx context.Context, desiredConfig, config *v1.Configuration) (bool, error) {

serving/pkg/reconciler/service/service.go

Line 249 in 4bfda35

func routeSemanticEquals(ctx context.Context, desiredRoute, route *v1.Route) (bool, error) {

But yes agree. The kmp.SafeDiff seems redundant. I will just add the labels DeepEqual + copying.

julz
julz reviewed Oct 7, 2020
View reviewed changes
pkg/reconciler/route/reconcile_resources.go Outdated Show resolved Hide resolved
pkg/reconciler/route/reconcile_resources.go Outdated Show resolved Hide resolved
@knative-prow-robot knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 7, 2020
julz
julz reviewed Oct 7, 2020
View reviewed changes
Copy link
Member

@julz julz left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2020
@codecov
Copy link

codecov bot commented Oct 7, 2020 •
edited
Loading

Codecov Report

Merging #9719 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9719      +/-   ##
==========================================
+ Coverage   87.94%   87.97%   +0.02%     
==========================================
  Files         177      177              
  Lines        8447     8449       +2     
==========================================
+ Hits         7429     7433       +4     
+ Misses        774      773       -1     
+ Partials      244      243       -1
Impacted Files Coverage Δ
pkg/reconciler/route/reconcile_resources.go 75.57% <100.00%> (+0.37%) ⬆️
pkg/reconciler/autoscaling/kpa/scaler.go 88.57% <0.00%> (-1.43%) ⬇️
pkg/autoscaler/statforwarder/forwarder.go 84.11% <0.00%> (+1.17%) ⬆️
pkg/reconciler/configuration/configuration.go 89.16% <0.00%> (+1.66%) ⬆️

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 4bfda35...668dc98. Read the comment docs.

@knative-test-reporter-robot
Copy link

The following jobs failed:

Test name Triggers Retries
pull-knative-serving-upgrade-tests 2020-10-07 13:42:13.681 +0000 UTC 1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-upgrade-tests

@knative-prow-robot knative-prow-robot merged commit 6f5e1cc into knative:master Oct 7, 2020
@nak3 nak3 deleted the ingres-label-reconcile branch October 7, 2020 14:38
nak3 added a commit to nak3/serving that referenced this pull request Oct 8, 2020
@nak3
[RELEASE-0.17] Reconcile Ingress when label was different from desired (
b074e5b 
knative#9719)

* Reconcile Ingress when label was different from desired

As per subject, this patch changes to reconcile Ingress when
it was different from desired.
knative-prow-robot pushed a commit that referenced this pull request Oct 13, 2020
@nak3
[RELEASE-0.17] Reconcile Ingress when label was different from desired (
4ed85f3 
#9719) (#9730)

* Reconcile Ingress when label was different from desired

As per subject, this patch changes to reconcile Ingress when
it was different from desired.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julz julz julz left review comments

@markusthoemmes markusthoemmes markusthoemmes left review comments

@mattmoor mattmoor Awaiting requested review from mattmoor

@tcnghia tcnghia Awaiting requested review from tcnghia

Assignees

@julz julz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/networking cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nak3 @knative-prow-robot @knative-test-reporter-robot @julz @markusthoemmes