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

Run action on different platforms #65

Merged
merged 1 commit into from
Aug 3, 2020
Merged

Run action on different platforms #65

merged 1 commit into from
Aug 3, 2020

Conversation

SVilgelm
Copy link
Member

@SVilgelm SVilgelm commented Aug 2, 2020

Closes #24

  • Added the matrix in test workflow to run on ubuntu, macOS and windows
  • Changed the installLint function to modify the AssetURL to download platform specifying binary and use proper extractor (.tar.gz and .zip)
  • Changed the cache functions to support the platform specific paths

@SVilgelm SVilgelm force-pushed the platforms branch 10 times, most recently from cce3e22 to 89b7eef Compare August 2, 2020 03:29
@SVilgelm SVilgelm requested review from jirfag, a team, sayboras and ernado August 2, 2020 03:35
@SVilgelm
Copy link
Member Author

SVilgelm commented Aug 2, 2020

@golangci/core-team @ernado @jirfag Could you please review this PR and if you are OK then please change the branch protection with new status checks

Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Couple of comments for my understanding.

src/cache.ts Outdated
@@ -34,7 +39,7 @@ const getIntervalKey = (invalidationIntervalDays: number): string => {

async function buildCacheKeys(): Promise<string[]> {
const keys = []
let cacheKey = `golangci-lint.cache-`
let cacheKey = util.format("golangci-lint.cache-%s-%s-", os.platform(), os.arch())
Copy link
Member

Choose a reason for hiding this comment

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

just curious why cache key needs to be updated ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it might be mixed up with caches for different platforms, but looks like everything works fine. Reverted back

src/install.ts Outdated Show resolved Hide resolved
src/install.ts Outdated
}
const s = util.format("%s-%s.%s", platform, arch, ext)
if (s != defaultPlatform) {
return versionConfig.AssetURL.replace(defaultPlatform, s)
Copy link
Member

Choose a reason for hiding this comment

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

understand that you are using AssetURL to cope with existing logic. I am thinking about using TargetVersion and then construct URL based on version. Then later, we can just remove redundant AssetURL in golangci-lint.

Anyway, I am feel free to ignore this comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

to get rid of the AssetURL we need to came up with more complex logic of finding the latest version and latest patch version. Let's use it for now

Copy link
Member Author

Choose a reason for hiding this comment

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

I see what you mean, yes, we can use TargetVersion to build the AssetURL

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@hu13
Copy link

hu13 commented Aug 2, 2020

@SVilgelm yes, i tested golangci/golangci-lint-action@platforms, the action downloaded the right binary for macOS's VM and the linter works.
Thanks

@SVilgelm SVilgelm linked an issue Aug 2, 2020 that may be closed by this pull request
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for making awesome changes 👍

@SVilgelm
Copy link
Member Author

SVilgelm commented Aug 3, 2020

@sayboras I already know how to retire the assets :) But it will take some time to implement, I'm newbie in typescript, we just need to use GitHub API to request the list of releases instead of assets and do same with sorting, but we need to be careful with the performance, It can be slower than using the assets, but will see

@ernado
Copy link
Member

ernado commented Aug 3, 2020

please change the branch protection with new status checks

What should be exactly done?

@SVilgelm
Copy link
Member Author

SVilgelm commented Aug 3, 2020

@ernado since I changed the os matrix the name of status check were caged as well, and there is no test status check anymore. Could you remove check from the test and set a check for the build-and-test / test in the branch protection of the master branch?

@ernado ernado merged commit 6317259 into master Aug 3, 2020
@SVilgelm SVilgelm deleted the platforms branch August 3, 2020 14:36
@ernado
Copy link
Member

ernado commented Aug 3, 2020

image

Now I'm not sure what to enable. Can't find "build-and-test" here.

@SVilgelm
Copy link
Member Author

SVilgelm commented Aug 3, 2020

@ernado looks like all 3 should be enabled: test (*)

@ldez ldez added the enhancement New feature or request label Apr 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can this run on macos-latest VM? Downloads linux binary for windows build
5 participants