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

Implement checkImageChange and tests #5333

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

Warashi
Copy link
Contributor

@Warashi Warashi commented Nov 13, 2024

What this PR does:

Implement checkImageChange and tests.
The checkImageChange detects changed images from the difference of manifests.

Why we need it:

We must detect whether there are changed images or not to determine the sync strategy and its descriptions.
If there are changed images in diffs, we choose the pipeline sync strategy.
If there are no changed images in diffs, we determine the sync strategy with other viewpoints.

Which issue(s) this PR fixes:

Part of #4980

Does this PR introduce a user-facing change?: No

  • How are users affected by this change:
  • Is this breaking change:
  • How to migrate (if breaking change):

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 25.31%. Comparing base (1763f92) to head (1112cd9).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5333      +/-   ##
==========================================
+ Coverage   25.28%   25.31%   +0.02%     
==========================================
  Files         444      444              
  Lines       47574    47592      +18     
==========================================
+ Hits        12028    12046      +18     
  Misses      34603    34603              
  Partials      943      943              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

`,
want: "Sync progressively because of updating image nginx from 1.19.3 to 1.19.4, image redis from 6.0.9 to 6.0.10",
wantOk: true,
},
Copy link
Member

@khanhtc1202 khanhtc1202 Nov 14, 2024

Choose a reason for hiding this comment

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

nits, Could you add a test case to show that the order of the image matters to this checkImageChange function 👀

{
			name: "change the order cause multi-image update",
			old: `
apiVersion: apps/v1
kind: Deployment
metadata:
  name: app-deployment
spec:
  template:
    spec:
      containers:
      - name: nginx
        image: nginx:1.19.3
      - name: redis
        image: redis:6.0.9
`,
			new: `
apiVersion: apps/v1
kind: Deployment
metadata:
  name: app-deployment
spec:
  template:
    spec:
      containers:
      - name: redis
        image: redis:6.0.9
      - name: nginx
        image: nginx:1.19.3
`,
			want:   "Sync progressively because of updating image nginx:1.19.3 to redis:6.0.9, image redis:6.0.9 to nginx:1.19.3",
			wantOk: true,
		},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I added it on this commit.
1112cd9

@khanhtc1202
Copy link
Member

LGTM with small nits 👍

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

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

LGTM

@Warashi Warashi merged commit 249315b into master Nov 14, 2024
16 of 18 checks passed
@Warashi Warashi deleted the pipedv1-k8s-plugin-check-image-change branch November 14, 2024 02:00
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.

3 participants