Open inancgumus opened 5 months ago
cc: @codebien
@codebien @inancgumus I've taken a look at this for the browser project. I think this is a great idea. Unfortunately the browser codebase deviates quite a bit from the k6 codebase, and therefore the k6 linter rules point out many issues in the browser code base:
It looks like the linter action defaults to only working with the k6 .golangci.yml
file. There are three possible solutions to help align the browser's codebase with k6's:
.golangci.yml
file. The issue here is that the defeats the whole point in working with the linter action and doesn't allow the browser to ever align with the k6 codebase. So, really this isn't an option, but i've added it to consider anyway.only-new-issues: true
to the golangci-lint
CI action after this line, which would allow all existing linter issues to exist, but lint new code changes.Personally I prefer the second option which is a good balance of aligning with the k6 linter quickly and then being able to slowly resolve all the linter issues over time.
WDYT?
@codebien @inancgumus apart from the lint, there are also the following jobs:
dependencies
test-go-versions
test-build
test-go-versions
isn't going to work with the browser codebase since we need to be able to add some env vars when running the test like we've done here. The possible solutions for this are:
test-go-versions
so that it accepts arguments.WDYT?
@ankur22 @codebien
Add only-new-issues: true to the golangci-lint CI action after this line, which would allow all existing linter issues to exist, but lint new code changes.
+1 for this. Other than aligning codebases, the primary idea of merging linters is to prevent security issues. For now, I believe it's good enough to cover future code changes with the k6-core linter integration. We'll eventually get to work on fixing the remaining linter issues for the existing code in time (which doesn't have security issues).
test-go-versions isn't going to work with the browser codebase since we need to be able to add some env vars when running the test like we've done here. The possible solutions for this are:
- Only work with the lint workflow for now, as long as we can resolve the issue detailed in this https://github.com/grafana/xk6-browser/issues/1292#issuecomment-2092665993.
- Amend test-go-versions so that it accepts arguments.
I'm fine with the second option if @grafana/k6-core is also fine with it. If not, I think the first option is good enough.
Add only-new-issues: true to the golangci-lint CI action after this line, which would allow all existing linter issues to exist, but lint new code changes.
This approach sounds the correct one for starting. :+1:
In any case, you should review the current cases to check if we have some with high priority. Maybe, we have to allocate someone from our team to support you on this review.
In any case, you should review the current cases to check if we have some with high priority. Maybe, we have to allocate someone from our team to support you on this review.
Agreed that the codebase should be audited by a k6-core team member. This I believe is the plan in any case before the browser module can be graduated out of experimental.
For this issue, i think it should be good enough to trust that all due diligence has been met so far, but with the belief that we will correctly align the codebase style and conform to the security standards ok k6, although we may need to make some exceptions for os
package usage.
What?
Why?
setInputFiles
.How?
k6-core prefers to use the same rules used in the linter configuration to sync across the projects (or extensions in general). k6-core has created a reusable workflow that they've been using for other minor extensions, as well. A concrete example is related to the
os
package which is denied in the k6-core configuration.They suggest k6-browser to use the entire workflow here for example. Or, only the linter configuration part. They believe a better case would be the ability to merge two golangci configurations, but they don't find it as a solution at the moment.
Tasks