-
Notifications
You must be signed in to change notification settings - Fork 240
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
setup GitHub Actions #243
setup GitHub Actions #243
Conversation
.github/workflows/main.yml
Outdated
run: | | ||
if [[ "${{ matrix.lang }}" == "API" ]]; then | ||
export TEST="API"; | ||
export COVERALLS_REPO_TOKEN="COVERALLS TOKEN FOR M2CGEN"; |
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.
FIXME
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.
I'm not quite sure how to overcome this. It doesn't seem to be a good idea to expose the API token and make it a part of the publicly available source code. Or am I missing something?
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.
Shall we use a Coveralls action (https://github.com/marketplace/actions/coveralls-github-action) instead of manually invoking the coveralls
binary in the test.sh
script?
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.
API tokens are exposed in a secure way via encoded values.
Very good idea to migrate to Coveralls action!
Anyway, for this PR I believe it's better simply disable Coveralls command at GitHub Actions. Done in 12147b7.
export LC_ALL="en_US.UTF-8"; | ||
sudo locale-gen $LC_ALL; | ||
sudo update-locale; |
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.
Workaround for
Process terminated. Infinite recursion during resource lookup within System.Private.CoreLib. This may be a bug in System.Private.CoreLib, or potentially in certain extensibility points such as assembly resolve events or CultureInfo names. Resource name: Argument_InvalidResourceCultureName
at System.Environment.FailFast(System.String)
at System.SR.InternalGetResourceString(System.String)
at System.SR.GetResourceString(System.String, System.String)
at System.Globalization.CultureInfo.VerifyCultureName(System.String, Boolean)
at System.Resources.ResourceManager.GetResourceFileName(System.Globalization.CultureInfo)
at System.Resources.ManifestBasedResourceGroveler.GrovelForResourceSet(System.Globalization.CultureInfo, System.Collections.Generic.Dictionary`2<System.String,System.Resources.ResourceSet>, Boolean, Boolean)
at System.Resources.ResourceManager.InternalGetResourceSet(System.Globalization.CultureInfo, Boolean, Boolean)
at System.Resources.ResourceManager.GetString(System.String, System.Globalization.CultureInfo)
at System.SR.InternalGetResourceString(System.String)
at System.SR.GetResourceString(System.String, System.String)
at System.Globalization.CultureInfo.VerifyCultureName(System.String, Boolean)
at System.Resources.ResourceManager.GetResourceFileName(System.Globalization.CultureInfo)
at System.Resources.ManifestBasedResourceGroveler.GrovelForResourceSet(System.Globalization.CultureInfo, System.Collections.Generic.Dictionary`2<System.String,System.Resources.ResourceSet>, Boolean, Boolean)
at System.Resources.ResourceManager.InternalGetResourceSet(System.Globalization.CultureInfo, Boolean, Boolean)
at System.Resources.ResourceManager.GetString(System.String, System.Globalization.CultureInfo)
at Microsoft.DotNet.Configurer.DotnetFirstTimeUseConfigurer.PrintFirstTimeMessageWelcome()
at Microsoft.DotNet.Configurer.DotnetFirstTimeUseConfigurer.Configure()
at Microsoft.DotNet.Cli.Program.ConfigureDotNetForFirstTimeUse(Microsoft.DotNet.Configurer.IFirstTimeUseNoticeSentinel, Microsoft.DotNet.Configurer.IAspNetCertificateSentinel, Microsoft.DotNet.Configurer.IFileSentinel, Boolean, Microsoft.DotNet.Configurer.DotnetFirstRunConfiguration, Microsoft.DotNet.Cli.Utils.IEnvironmentProvider)
at Microsoft.DotNet.Cli.Program.ProcessArgs(System.String[], Microsoft.DotNet.Cli.Telemetry.ITelemetry)
at Microsoft.DotNet.Cli.Program.Main(System.String[])
.github/workflows/main.yml
Outdated
run: | | ||
if [[ "${{ matrix.lang }}" == "API" ]]; then | ||
export TEST="API"; | ||
export COVERALLS_REPO_TOKEN="COVERALLS TOKEN FOR M2CGEN"; |
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.
Shall we use a Coveralls action (https://github.com/marketplace/actions/coveralls-github-action) instead of manually invoking the coveralls
binary in the test.sh
script?
Oh, I just noticed that the Action with R language takes more than 2 hours (all other Actions take approximately the same time as they do at Travis)! It looks like either Travis has better CPUs or Rscript conflicts with some installed software in GitHub Actions image. |
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.
Looks great, thank you! What is our plan with regard to Coveralls and moving away from Travis completely?
I'm pretty sure we should use Coveralls Action instead of manual invocation in bash script. But unfortunately, I have no experience with it. Speaking about the Travis, the only reason I see to keep it is even more parallel jobs (possibly later with greater number of supported languages and models). Maybe we can run |
That's actually a very interesting idea, tough I'd rather prefer to have a consistent and uniform environment for all our tests. We should keep this in mind.
I'll take a look into this one |
So, we are removing Travis config for now (after migrating to Coveralls Action), right? Also, please don't forget to make GitHub Actions checks required: #243 (comment). One more frustrating thing is that each time config (matrix or jobs name) changes, you need to go to repo settings and update the list of required checks. Refer to microsoft/LightGBM#3188 (comment) (paragraph starting with |
Yeah, let's do that. |
BTW, we can use our Docker file for that purpose. |
Hmm, seems that Actions can be triggered even without config in the
master
branch:One more step is to make passing tests at GitHub Actions for PRs required. Refer to microsoft/LightGBM#3119 (comment)