search

mattermost / circleci-orbs

Orbs used for building in MM repositories on circleci
3 stars 9 forks source link

Upgrade golangci-lint #20

Closed cpoile closed 4 years ago

cpoile commented 4 years ago

Summary

cpoile commented 4 years ago

@hanzei We can use a parameter for the job. I'm pretty sure this will work, but I'm not sure how to test this without pushing. I can't find a solution online for using an orb from a PR's hash, do you know?

lieut-data commented 4 years ago

I'd be inclined to fix these issues post merge -- if the plugins aren't broken on 1.24.0 now, they'll probably start failing that way anyway. If we can't update the version centrally here, I'd argue we shouldn't be sharing the orbs in the first place since we'll be restricted by the lowest common denominator. Presumably plugins that choose not to fix can just pin their orb as required?

hanzei commented 4 years ago

I'd be inclined to fix these issues post merge -- if the plugins aren't broken on 1.24.0 now, they'll probably start failing that way anyway. If we can't update the version centrally here, I'd argue we shouldn't be sharing the orbs in the first place since we'll be restricted by the lowest common denominator. Presumably plugins that choose not to fix can just pin their orb as required?

@lieut-data I agree that we are bound to the lowest common denominator. But for historical reasons, all plugins use the latest version, which might not be a good idea.

The pain point is that we will have to touch multiple plugins after the version bump. We can bump the version when we ensure every CI pipeline gets fixed soon after the bump.

lieut-data commented 4 years ago

Thanks for the extra context, @hanzei. Right now, we've had to disable golangci-lint manually for incident response, but I'd love to get that restored. I'm not a huge fan of parameterizing this property instead of using the existing "orb version" parameter, since it feels like a workaround. If we're committed to this approach, it might be easier for us to just "fork" the orb for our own purposes.

Thoughts on next steps? My gut says to either merge this and deal with the fallout (possibly by just pinning), or just abandon this PR altogether.

hanzei commented 4 years ago

I'm fine with merging and dealing with the consequences :+1:

hanzei commented 4 years ago

@cpoile Would you mind reverting back to https://github.com/mattermost/circleci-orbs/pull/20/commits/bdff182d9ac67ddd75236ab44580867cb0a06bdb?

lieut-data commented 4 years ago

@hanzei, is QA review required here? Or do we just treat this as developer/build related?

hanzei commented 4 years ago

Getting QA review for these kind of PRs is just not worth it

hanzei commented 4 years ago

I've submitted three PRs to fix the linter issues:

Please take a look at them so we can merge them and this PR soon

lieut-data commented 4 years ago

You're the best, @hanzei! I've approved those PRs -- very light touches, which is good!

hanzei commented 4 years ago

@cpoile Is this PR ready to get merged?