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

修复manifestEqual方法缺陷 #159

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

veraicon
Copy link

Pull Request Description

Describe what this PR does / why we need it

修复manifestEqual方法缺陷。
该方法的意图应当是对比来源和目标镜像的manifest,如果相同就skip该任务。但是该方法之前在本地和远程manifest内容实际上完全一致,但是key顺序不同的情况下,会返回false,没有成功比较。
而由于destManifest是本地构造的,因此key顺序大概率与远程不同。
这导致重复向远程仓库提交push;而在远程仓库开启immutable的情况下,会导致任务预期外的失败。

Does this pull request fix one issue?

是的,已测试

Describe how you did it

将简单的json.compact成为无序byte数组的比较,转变成递归的key value逐个值比较,包括slice。

Describe how to verify it

将目标镜像仓库设置immutable,也就是已存在的tag不允许重复提交;此时将来源和目标设置为相同地址进行提交,在原始版本会报错。
unknown: The requested tag already exists and cannot be overwritten
当前提交版本会提示 skip synchronization because destination image exists 并返回成功

Special notes for reviews

…成功比较。而由于destManifest是本地构造的,因此key顺序大概率与远程不同。这导致重复向远程仓库提交push;而在远程仓库开启immutable的情况下,会导致任务预期外的失败。
@veraicon
Copy link
Author

举例来说,根据日志,之前的版本会对比如下两个str,并返回false,实际上他们的内容值是一致的(这里对digest进行了mask):
{
"mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
"schemaVersion": 2,
"manifests": [
{
"mediaType": "application/vnd.docker.distribution.manifest.v2+json",
"digest": "sha256:",
"size": 2831,
"platform": {
"architecture": "arm64",
"os": "linux"
}
},
{
"mediaType": "application/vnd.docker.distribution.manifest.v2+json",
"digest": "sha256:
",
"size": 2831,
"platform": {
"architecture": "amd64",
"os": "linux"
}
}
]
}

{"schemaVersion":2,"mediaType":"application/vnd.docker.distribution.manifest.list.v2+json","manifests":[{"mediaType":"application/vnd.docker.distribution.manifest.v2+json","size":2831,"digest":"sha256:","platform":{"architecture":"arm64","os":"linux"}},{"mediaType":"application/vnd.docker.distribution.manifest.v2+json","size":2831,"digest":"sha256:","platform":{"architecture":"amd64","os":"linux"}}]}

Copy link
Contributor

@denverdino denverdino left a comment

Choose a reason for hiding this comment

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

        json.Unmarshal(m1, &a)
	json.Unmarshal(m2, &b)

Pls add error check for Unmarshal

And how about to use eq := reflect.DeepEqual(m1, m2)

More options for pkg maps and cmp in Golang 1.21

https://blog.stackademic.com/introducing-new-packages-in-go-maps-and-cmp-2639e64b6a85

@veraicon
Copy link
Author

        json.Unmarshal(m1, &a)
	json.Unmarshal(m2, &b)

Pls add error check for Unmarshal

And how about to use eq := reflect.DeepEqual(m1, m2)

More options for pkg maps and cmp in Golang 1.21

https://blog.stackademic.com/introducing-new-packages-in-go-maps-and-cmp-2639e64b6a85

Added unmarshal error handling and replaced custom implementation with deepEqual.
Pls review ,thanks.

@denverdino denverdino merged commit 20f6a24 into AliyunContainerService:master Aug 29, 2024
6 of 7 checks passed
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.

2 participants