Skip to content

platforms: fill default arm variant when parse platform specifier#3939

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
zhsj:fix-arm
Jan 8, 2020
Merged

platforms: fill default arm variant when parse platform specifier#3939
crosbymichael merged 1 commit intocontainerd:masterfrom
zhsj:fix-arm

Conversation

@zhsj
Copy link
Copy Markdown
Contributor

@zhsj zhsj commented Jan 7, 2020

arm has been supported, but something is missing, causes test failure

--- FAIL: TestParseSelector (0.00s)
    --- SKIP: TestParseSelector/* (0.00s)
        platforms_test.go:288: this case is not yet supported
    --- SKIP: TestParseSelector/linux/* (0.00s)
        platforms_test.go:288: this case is not yet supported
    --- SKIP: TestParseSelector/*/arm64 (0.00s)
        platforms_test.go:288: this case is not yet supported
    --- PASS: TestParseSelector/linux/arm64 (0.00s)
    --- PASS: TestParseSelector/linux/arm64/v8 (0.00s)
    --- PASS: TestParseSelector/linux/arm (0.00s)
    --- PASS: TestParseSelector/linux/arm/v6 (0.00s)
    --- PASS: TestParseSelector/linux/arm/v7 (0.00s)
    --- PASS: TestParseSelector/arm (0.00s)
    --- PASS: TestParseSelector/armel (0.00s)
    --- PASS: TestParseSelector/armhf (0.00s)
    --- PASS: TestParseSelector/Aarch64 (0.00s)
    --- PASS: TestParseSelector/x86_64 (0.00s)
    --- PASS: TestParseSelector/Linux/x86_64 (0.00s)
    --- PASS: TestParseSelector/i386 (0.00s)
    --- FAIL: TestParseSelector/linux (0.00s)
        platforms_test.go:292: arm support not fully implemented: not implemented
    --- PASS: TestParseSelector/s390x (0.00s)
    --- PASS: TestParseSelector/linux/s390x (0.00s)
    --- FAIL: TestParseSelector/macOS (0.00s)
        platforms_test.go:292: arm support not fully implemented: not implemented

The test like linux/arm/v6 and linux/arm/v7 have passed, so I don't think there's issue for arm actually.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jan 7, 2020

Build succeeded.

@zhsj
Copy link
Copy Markdown
Contributor Author

zhsj commented Jan 7, 2020 •
edited
Loading

Hmm, there's still problem is this patch.

I send it after I have tested it on arm7 platform, and it passed. But I find it failed on arm8.

    --- FAIL: TestParseSelector/linux (0.00s)
        platforms_test.go:296: platform did not match expected: v1.Platform{Architecture:"arm", OS:"linux", OSVersion:"", OSFeatures:[]string(nil), Variant:"v8"} != v1.Platform{Architecture:"arm", OS:"linux", OSVersion:"", OSFeatures:[]string(nil), Variant:""}
    --- FAIL: TestParseSelector/macOS (0.00s)
        platforms_test.go:296: platform did not match expected: v1.Platform{Architecture:"arm", OS:"darwin", OSVersion:"", OSFeatures:[]string(nil), Variant:"v8"} != v1.Platform{Architecture:"arm", OS:"darwin", OSVersion:"", OSFeatures:[]string(nil), Variant:""}

I think the modification in platforms/platforms.go is correct since it returns the expected "v8". But the test seems problematic... Any advice to write this test more robustly?..

@zhsj
Copy link
Copy Markdown
Contributor Author

zhsj commented Jan 8, 2020

I think the modification in platforms/platforms.go is correct since it returns the expected "v8". But the test seems problematic... Any advice to write this test more robustly?..

Looks a little ugly, added a defaultVariant var in test...

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jan 8, 2020

Build succeeded.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 8, 2020 •
edited
Loading

Codecov Report

Merging #3939 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3939      +/-   ##
=========================================
- Coverage   42.41%   42.4%   -0.02%     
=========================================
  Files         130     130              
  Lines       14738   14737       -1     
=========================================
- Hits         6251    6249       -2     
  Misses       7567    7567              
- Partials      920     921       +1
Flag Coverage Δ
#linux 45.77% <0%> (-0.02%) ⬇️
#windows 37.94% <0%> (ø) ⬆️
Impacted Files Coverage Δ
platforms/platforms.go 80.21% <0%> (+0.87%) ⬆️
snapshots/btrfs/btrfs.go 57.39% <0%> (-0.9%) ⬇️

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 0d276ec...90cd777. Read the comment docs.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jan 8, 2020

Build succeeded.

@zhsj zhsj mentioned this pull request Jan 8, 2020
Closed
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jan 8, 2020

Build succeeded.

@zhsj
platforms: fill default arm variant when parse platform specifier
90cd777 
arm has been supported, but something is missing, causes test failure

    --- FAIL: TestParseSelector/linux (0.00s)
        platforms_test.go:292: arm support not fully implemented: not implemented
    --- FAIL: TestParseSelector/macOS (0.00s)
        platforms_test.go:292: arm support not fully implemented: not implemented

Signed-off-by: Shengjing Zhu <[email protected]>
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jan 8, 2020

Build succeeded.

@zhsj
Copy link
Copy Markdown
Contributor Author

zhsj commented Jan 8, 2020

I think the PR is ready for review. The test has passed on my arm/v7 and arm/v8 machine, and the openlab's arm64 machine.

estesp
estesp approved these changes Jan 8, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Jan 8, 2020

LGTM

@crosbymichael crosbymichael merged commit 41088e4 into containerd:master Jan 8, 2020
@zhsj zhsj deleted the fix-arm branch January 8, 2020 16:04
@zhsj zhsj mentioned this pull request Jan 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estesp estesp estesp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zhsj @codecov-io @crosbymichael @estesp