-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
ci: gha test workflow for integration and unit test #44034
Conversation
https://github.com/moby/moby/runs/8017622176?check_suite_focus=true#step:6:1856
related #42484 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
integration-cli/requirements_test.go
Outdated
@@ -69,6 +69,10 @@ func UnixCli() bool { | |||
return isUnixCli | |||
} | |||
|
|||
func NotGitHubActions() bool { | |||
return os.Getenv("GITHUB_ACTIONS") != "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use strconv.ParseBool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we can either go with "allow the variable to both enable or explicitly disable", in which case we would have something like gzDecompress()
Or, we could keep it simple and consider any non-empty value to be "enabled", and empty to be "disabled";
Line 198 in 9184f0b
if os.Getenv("DOCKER_ALLOW_SCHEMA1_PUSH_DONOTUSE") == "" { |
Given that this is only for CI, and not a user-facing option, I'd be ok with "non-empty" means "enabled";
return os.Getenv("GITHUB_ACTIONS") != "true" | |
return os.Getenv("GITHUB_ACTIONS") == "" |
Or is it expected for this var to have GITHUB_ACTIONS=false
? I see various project just checking for "empty", but there's some that have "false" conditions as well; https://grep.app/search?q=GITHUB_ACTIONS&case=true&filter[lang][0]=Shell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non empty sgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cgroup2 mode seems misconfigured
Good catch, thanks Akihiro! |
LOL; looks like we need to hash the paths;
|
7c9071e
to
1aa0970
Compare
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
@AkihiroSuda @tianon PTAL 🤗 |
integration-cli/requirements_test.go
Outdated
@@ -69,6 +69,10 @@ func UnixCli() bool { | |||
return isUnixCli | |||
} | |||
|
|||
func GitHubActions() bool { | |||
return os.Getenv("GITHUB_ACTIONS") == "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strconv.ParseBool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, dang; I recall I commented on this to use "non-empty" (as I don't think we'll ever gonna use GITHUB_ACTIONS=false
); #44034 (comment)
Looks like it was inadvertently marked as "resolved"; does that suggestion work for you, @AkihiroSuda ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-empty also sgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I missed this one. Done with non-empty.
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
This one again 😞: https://github.com/moby/moby/runs/8246861287?check_suite_focus=true#step:6:225
@thaJeztah Could we merge #44086 first? This is really frustrating and seems to happen quite often. |
Thanks for the re-run @thaJeztah, good to merge? |
Thanks! I think that should address @AkihiroSuda's comment; let me merge this to unblock the other work, but @AkihiroSuda if there's anything to fix up after this, let me know ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
follow-up #43479
- What I did
Adds gha
test
workflow to run integration and unit test. Will ease testing #43529 and also moving out some bits from Jenkins.I use the same matrix strategy to distribute integration-cli tests as windows workflow.
Looking at the overall number of jobs running in the pipeline now I think we could remove tests againstwindows-2019
so queue size would be reduce.Some tests look broken on GitHub Runner, probably linked to the runner environment so skipping them on gha env for now.
integration (rootless)
: https://github.com/crazy-max/moby/runs/8010866754?check_suite_focus=true#step:7:1032related #41561
ìntegration-cli
: https://github.com/crazy-max/moby/runs/8010867286?check_suite_focus=true#step:6:662related #36541
ìntegration-cli
: https://github.com/crazy-max/moby/runs/8010867670?check_suite_focus=true#step:6:1328related #41559 #43996
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)