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

Migrating to k6-core's linter #1292

Open
4 tasks
Tracked by #1117
inancgumus opened this issue May 2, 2024 · 7 comments
Open
4 tasks
Tracked by #1117

Migrating to k6-core's linter #1292

inancgumus opened this issue May 2, 2024 · 7 comments
Labels
ci compatibility k6 core compatibility security

Comments

@inancgumus
Copy link
Member

inancgumus commented May 2, 2024

What?

k6-core and k6-browser linter configurations do not match. k6-core wonders if we can use their version directly or if it would be better to use it as the base and extend it with k6-browser's additional rules.

Why?

To rely on a shared and reviewed configuration across teams to prevent security issues like 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

Tasks

Related PR(s)/Issue(s)

No response

@inancgumus inancgumus added ci compatibility k6 core compatibility security labels May 2, 2024
@inancgumus
Copy link
Member Author

cc: @codebien

@ankur22 ankur22 self-assigned this May 2, 2024
@ankur22
Copy link
Collaborator

ankur22 commented May 3, 2024

@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:

linter errors

link to job: https://github.com/grafana/xk6-browser/actions/runs/8937169532/job/24548895220?pr=1297

  Error: declaration has 4 blank identifiers (dogsled)
  Error: declaration has 4 blank identifiers (dogsled)
  Error: Error return value of `p.Reload` is not checked (errcheck)
  Error: Error return value of `p.Reload` is not checked (errcheck)
  Error: Error return value of `(*github.com/grafana/xk6-browser/common.Locator).Type` is not checked (errcheck)
  Error: use of `os.LookupEnv` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `logrus.Logger` forbidden by pattern `^logrus\.Logger$` (forbidigo)
  Error: use of `logrus.Logger` forbidden by pattern `^logrus\.Logger$` (forbidigo)
  Error: use of `os.MkdirAll` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.OpenFile` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.MkdirTemp` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.RemoveAll` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.WriteFile` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.Stat` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.ReadFile` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.TempDir` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.MkdirTemp` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.RemoveAll` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.FindProcess` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.IsNotExist` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.Process` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.Process` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `logrus.Logger` forbidden by pattern `^logrus\.Logger$` (forbidigo)
  Error: use of `logrus.Logger` forbidden by pattern `^logrus\.Logger$` (forbidigo)
  Error: use of `os.PathSeparator` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.MkdirTemp` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.RemoveAll` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.MkdirTemp` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.RemoveAll` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.FindProcess` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.ErrProcessDone` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: use of `os.PathSeparator` forbidden because "Using anything except Signal and SyscallError from the os package is forbidden" (forbidigo)
  Error: Function 'initUS' is too long (383 > 80) (funlen)
  Error: Function 'WithCDPHandler' is too long (86 > 80) (funlen)
  Error: Function 'Parse' has too many statements (70 > 60) (funlen)
  Error: Function 'recvLoop' is too long (110 > 80) (funlen)
  Error: Function 'GetDevices' is too long (783 > 80) (funlen)
  Error: Function 'eval' is too long (88 > 80) (funlen)
  Error: Function 'frameNavigated' is too long (104 > 80) (funlen)
  Error: Function 'initEvents' is too long (81 > 80) (funlen)
  Error: cognitive complexity 44 of func `mapPage` is high (> 30) (gocognit)
  Error: cognitive complexity 36 of func `mapFrame` is high (> 30) (gocognit)
  Error: cognitive complexity 33 of func `mapBrowserContext` is high (> 30) (gocognit)
  Error: singleCaseSwitch: found switch with default case only (gocritic)
  Error: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
  Error: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
  Error: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
  Error: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
  Error: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
  Error: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
  Error: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
  Error: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
  Error: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
  Error: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
  Error: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
  Error: wrapperFunc: use strings.ReplaceAll method in `strings.Replace(val, "n", "", -1)` (gocritic)
  Error: ifElseChain: rewrite if-else to switch statement (gocritic)
  Error: ifElseChain: rewrite if-else to switch statement (gocritic)
  Error: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
  Error: regexpMust: for const patterns like re, use regexp.MustCompile (gocritic)
  Error: line is 121 characters (lll)
  Error: line is 134 characters (lll)
  Error: line is 155 characters (lll)
  Error: line is 124 characters (lll)
  Error: line is 147 characters (lll)
  Error: line is 144 characters (lll)
  Error: line is 130 characters (lll)
  Error: line is 132 characters (lll)
  Error: line is 131 characters (lll)
  Error: line is 124 characters (lll)
  Error: line is 126 characters (lll)
  Error: line is 125 characters (lll)
  Error: line is 138 characters (lll)
  Error: line is 138 characters (lll)
  Error: line is 124 characters (lll)
  Error: line is 124 characters (lll)
  Error: line is 154 characters (lll)
  Error: line is 154 characters (lll)
  Error: line is 154 characters (lll)
  Error: line is 154 characters (lll)
  Error: line is 154 characters (lll)
  Error: line is 154 characters (lll)
  Error: line is 151 characters (lll)
  Error: line is 151 characters (lll)
  Error: line is 145 characters (lll)
  Error: line is 145 characters (lll)
  Error: line is 145 characters (lll)
  Error: line is 145 characters (lll)
  Error: line is 145 characters (lll)
  Error: line is 145 characters (lll)
  Error: line is 154 characters (lll)
  Error: line is 154 characters (lll)
  Error: line is 154 characters (lll)
  Error: line is 154 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 154 characters (lll)
  Error: line is 154 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 128 characters (lll)
  Error: line is 128 characters (lll)
  Error: line is 151 characters (lll)
  Error: line is 151 characters (lll)
  Error: line is 183 characters (lll)
  Error: line is 183 characters (lll)
  Error: line is 179 characters (lll)
  Error: line is 179 characters (lll)
  Error: line is 179 characters (lll)
  Error: line is 146 characters (lll)
  Error: line is 146 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 150 characters (lll)
  Error: line is 150 characters (lll)
  Error: line is 162 characters (lll)
  Error: line is 162 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 152 characters (lll)
  Error: line is 162 characters (lll)
  Error: line is 162 characters (lll)
  Error: line is 145 characters (lll)
  Error: line is 145 characters (lll)
  Error: line is 129 characters (lll)
  Error: line is 129 characters (lll)
  Error: line is 124 characters (lll)
  Error: line is 124 characters (lll)
  Error: line is 159 characters (lll)
  Error: line is 159 characters (lll)
  Error: line is 164 characters (lll)
  Error: line is 164 characters (lll)
  Error: line is 150 characters (lll)
  Error: line is 133 characters (lll)
  Error: line is 141 characters (lll)
  Error: line is 122 characters (lll)
  Error: line is 138 characters (lll)
  Error: line is 128 characters (lll)
  Error: line is 150 characters (lll)
  Error: line is 139 characters (lll)
  Error: line is 122 characters (lll)
  Error: line is 123 characters (lll)
  Error: line is 147 characters (lll)
  Error: line is 132 characters (lll)
  Error: line is 135 characters (lll)
  Error: `if opts.FullPage` has complex nested blocks (complexity: 11) (nestif)
  Error: `if msg.Method == cdproto.EventTargetAttachedToTarget` has complex nested blocks (complexity: 7) (nestif)
  Error: `if opts != nil && !goja.IsUndefined(opts) && !goja.IsNull(opts)` has complex nested blocks (complexity: 5) (nestif)
  Error: `if opts != nil && !goja.IsUndefined(opts) && !goja.IsNull(opts)` has complex nested blocks (complexity: 5) (nestif)
  Error: `if !fitsViewport` has complex nested blocks (complexity: 5) (nestif)
  Warning: exported: exported type KeyInput should have comment or be unexported (revive)
  Warning: exported: exported type KeyDefinition should have comment or be unexported (revive)
  Warning: exported: exported type KeyboardLayout should have comment or be unexported (revive)
  Warning: exported: exported type Logger should have comment or be unexported (revive)
  Warning: unused-parameter: parameter 'conn' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: unused-parameter: parameter 'file' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: var-naming: struct field HttpCredentials should be HTTPCredentials (revive)
  Warning: exported: exported function WithHooks should have comment or be unexported (revive)
  Warning: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: unused-parameter: parameter 'conn' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: unused-parameter: parameter 'data' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: exported: exported type ElementHandleBaseOptions should have comment or be unexported (revive)
  Warning: unused-parameter: parameter 'apiCtx' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: var-declaration: should omit type float64 from declaration of var value; it will be inferred from the right-hand side (revive)
  Warning: exported: exported type FrameBaseOptions should have comment or be unexported (revive)
  Warning: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: exported: exported type KeyboardOptions should have comment or be unexported (revive)
  Warning: exported: exported type BigIntParseError should have comment or be unexported (revive)
  Warning: var-declaration: should omit type float64 from declaration of var fromX; it will be inferred from the right-hand side (revive)
  Warning: empty-block: this block is empty, you can remove it (revive)
  Warning: var-declaration: should omit type *regexp.Regexp from declaration of var reQueryEngine; it will be inferred from the right-hand side (revive)
  Warning: exported: exported type Worker should have comment or be unexported (revive)
  Warning: exported: exported type BrowserProcess should have comment or be unexported (revive)
  Warning: unused-parameter: parameter 'event' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: exported: exported const EventBrowserDisconnected should have comment (or a comment on this block) or be unexported (revive)
  Warning: unused-parameter: parameter 'k' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: exported: exported const DefaultLocale should have comment (or a comment on this block) or be unexported (revive)
  Warning: exported: exported type HookID should have comment or be unexported (revive)
  Warning: var-declaration: should drop = nil from declaration of var redirectChain; it is the zero value (revive)
  Warning: exported: exported const BrowserStateOpen should have comment (or a comment on this block) or be unexported (revive)
  Warning: unused-parameter: parameter 'conn' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: exported: exported const ModifierKeyAlt should have comment (or a comment on this block) or be unexported (revive)
  Warning: exported: exported type PageEmulateMediaOptions should have comment or be unexported (revive)
  Warning: exported: exported function GetHooks should have comment or be unexported (revive)
  Warning: exported: exported type Barrier should have comment or be unexported (revive)
  Warning: exported: exported type ElementHandleBasePointerOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameCheckOptions should have comment or be unexported (revive)
  Warning: exported: exported function NewKeyboardOptions should have comment or be unexported (revive)
  Warning: var-declaration: should omit type float64 from declaration of var fromY; it will be inferred from the right-hand side (revive)
  Warning: exported: exported type UnserializableValueError should have comment or be unexported (revive)
  Warning: var-declaration: should omit type *regexp.Regexp from declaration of var reXPathSelector; it will be inferred from the right-hand side (revive)
  Warning: unused-parameter: parameter 'data' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: unused-parameter: parameter 'initial' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: unused-parameter: parameter 'lo' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: unused-parameter: parameter 'handle' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: var-declaration: should drop = nil from declaration of var frame; it is the zero value (revive)
  Warning: exported: exported const HookApplySlowMo should have comment (or a comment on this block) or be unexported (revive)
  Warning: unused-parameter: parameter 'apiCtx' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: exported: exported type PageReloadOptions should have comment or be unexported (revive)
  Warning: exported: exported function NewBarrier should have comment or be unexported (revive)
  Warning: exported: exported type ElementHandleCheckOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameClickOptions should have comment or be unexported (revive)
  Warning: exported: exported type MouseClickOptions should have comment or be unexported (revive)
  Warning: exported: exported method KeyboardOptions.Parse should have comment or be unexported (revive)
  Warning: unused-parameter: parameter 'x' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: var-naming: var backendNodeId should be backendNodeID (revive)
  Warning: exported: exported type SelectorPart should have comment or be unexported (revive)
  Warning: unused-parameter: parameter 'apiCtx' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: unused-parameter: parameter 'apiCtx' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: exported: exported type Hook should have comment or be unexported (revive)
  Warning: exported: exported method Barrier.AddFrameNavigation should have comment or be unexported (revive)
  Warning: exported: exported type PageScreenshotOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameDblclickOptions should have comment or be unexported (revive)
  Warning: exported: exported type ElementHandleClickOptions should have comment or be unexported (revive)
  Warning: exported: exported type MouseDblClickOptions should have comment or be unexported (revive)
  Warning: var-naming: method updateHttpCredentials should be updateHTTPCredentials (revive)
  Warning: unused-parameter: parameter 'apiCtx' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: exported: exported type Selector should have comment or be unexported (revive)
  Warning: exported: exported type Hooks should have comment or be unexported (revive)
  Warning: unused-parameter: parameter 'data' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: exported: exported method Barrier.Wait should have comment or be unexported (revive)
  Warning: exported: exported type ElementHandleDblclickOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameFillOptions should have comment or be unexported (revive)
  Warning: exported: exported type MouseDownUpOptions should have comment or be unexported (revive)
  Warning: unused-parameter: parameter 'apiCtx' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: unused-parameter: parameter 'x' seems to be unused, consider removing or renaming it as _ (revive)
  Warning: exported: exported method Page.EmulateMedia should have comment or be unexported (revive)
  Warning: exported: exported function NewSelector should have comment or be unexported (revive)
  Warning: var-naming: var frameId should be frameID (revive)
  Warning: exported: exported type DocumentInfo should have comment or be unexported (revive)
  Warning: exported: exported function NewHooks should have comment or be unexported (revive)
  Warning: exported: exported method PageEmulateMediaOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported type FrameGotoOptions should have comment or be unexported (revive)
  Warning: exported: exported type ElementHandleHoverOptions should have comment or be unexported (revive)
  Warning: exported: exported type MouseMoveOptions should have comment or be unexported (revive)
  Warning: exported: exported method Page.Fill should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandle.Dblclick should have comment or be unexported (revive)
  Warning: exported: exported method Hooks.Get should have comment or be unexported (revive)
  Warning: exported: exported type FrameHoverOptions should have comment or be unexported (revive)
  Warning: exported: exported function NewPageReloadOptions should have comment or be unexported (revive)
  Warning: exported: exported type ElementHandlePressOptions should have comment or be unexported (revive)
  Warning: exported: exported method Page.Focus should have comment or be unexported (revive)
  Warning: exported: exported function NewMouseClickOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandle.Fill should have comment or be unexported (revive)
  Warning: exported: exported method Mouse.DblClick should have comment or be unexported (revive)
  Warning: exported: exported type FrameInnerHTMLOptions should have comment or be unexported (revive)
  Warning: exported: exported method Hooks.Register should have comment or be unexported (revive)
  Warning: exported: exported method PageReloadOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported method Page.Hover should have comment or be unexported (revive)
  Warning: exported: exported type ElementHandleScreenshotOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandle.InputValue should have comment or be unexported (revive)
  Warning: exported: exported method MouseClickOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported type FrameInnerTextOptions should have comment or be unexported (revive)
  Warning: exported: exported method Page.InnerHTML should have comment or be unexported (revive)
  Warning: exported: exported function NewPageScreenshotOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandle.Press should have comment or be unexported (revive)
  Warning: exported: exported type ElementHandleSetCheckedOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameInputValueOptions should have comment or be unexported (revive)
  Warning: exported: exported method MouseClickOptions.ToMouseDownUpOptions should have comment or be unexported (revive)
  Warning: exported: exported method PageScreenshotOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported method Page.InnerText should have comment or be unexported (revive)
  Warning: exported: exported type ElementHandleTapOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandle.ScrollIntoViewIfNeeded should have comment or be unexported (revive)
  Warning: exported: exported function NewMouseDblClickOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameIsCheckedOptions should have comment or be unexported (revive)
  Warning: exported: exported method Page.InputValue should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandle.SelectOption should have comment or be unexported (revive)
  Warning: exported: exported type ElementHandleTypeOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameIsDisabledOptions should have comment or be unexported (revive)
  Warning: exported: exported method MouseDblClickOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandle.SelectText should have comment or be unexported (revive)
  Warning: exported: exported method Page.IsClosed should have comment or be unexported (revive)
  Warning: exported: exported type FrameIsEditableOptions should have comment or be unexported (revive)
  Warning: exported: exported type ElementHandleWaitForElementStateOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandle.TextContent should have comment or be unexported (revive)
  Warning: exported: exported function NewMouseDownUpOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameIsEnabledOptions should have comment or be unexported (revive)
  Warning: exported: exported method Page.IsDisabled should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandle.WaitForElementState should have comment or be unexported (revive)
  Warning: exported: exported function NewElementHandleBaseOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameIsHiddenOptions should have comment or be unexported (revive)
  Warning: exported: exported method MouseDownUpOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported method Page.IsEditable should have comment or be unexported (revive)
  Warning: exported: exported type FrameIsVisibleOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandleBaseOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported method Page.IsEnabled should have comment or be unexported (revive)
  Warning: exported: exported function NewMouseMoveOptions should have comment or be unexported (revive)
  Warning: exported: exported function NewElementHandleBasePointerOptions should have comment or be unexported (revive)
  Warning: exported: exported type FramePressOptions should have comment or be unexported (revive)
  Warning: exported: exported method MouseMoveOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported method Page.Press should have comment or be unexported (revive)
  Warning: exported: exported type FrameSelectOptionOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandleBasePointerOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported method Page.SelectOption should have comment or be unexported (revive)
  Warning: exported: exported function NewElementHandleCheckOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameSetContentOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandleCheckOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported method Page.SetContent should have comment or be unexported (revive)
  Warning: exported: exported function NewElementHandleClickOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameTapOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandleClickOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported method Page.TextContent should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandleClickOptions.ToMouseClickOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameTextContentOptions should have comment or be unexported (revive)
  Warning: exported: exported function NewElementHandleDblclickOptions should have comment or be unexported (revive)
  Warning: exported: exported method Page.Title should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandleDblclickOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported type FrameTypeOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandleDblclickOptions.ToMouseClickOptions should have comment or be unexported (revive)
  Warning: exported: exported method Page.Type should have comment or be unexported (revive)
  Warning: exported: exported function NewElementHandleHoverOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameUncheckOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandleHoverOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewElementHandlePressOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameWaitForFunctionOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameWaitForLoadStateOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandlePressOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandlePressOptions.ToBaseOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameWaitForNavigationOptions should have comment or be unexported (revive)
  Warning: exported: exported type FrameWaitForSelectorOptions should have comment or be unexported (revive)
  Warning: exported: exported function NewElementHandleScreenshotOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandleScreenshotOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewFrameBaseOptions should have comment or be unexported (revive)
  Warning: exported: exported method FrameBaseOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewElementHandleSetCheckedOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandleSetCheckedOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewFrameCheckOptions should have comment or be unexported (revive)
  Warning: exported: exported method FrameCheckOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewElementHandleTapOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandleTapOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewFrameClickOptions should have comment or be unexported (revive)
  Warning: exported: exported method FrameClickOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewElementHandleTypeOptions should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandleTypeOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewFrameDblClickOptions should have comment or be unexported (revive)
  Warning: exported: exported method FrameDblclickOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandleTypeOptions.ToBaseOptions should have comment or be unexported (revive)
  Warning: exported: exported function NewElementHandleWaitForElementStateOptions should have comment or be unexported (revive)
  Warning: exported: exported function NewFrameFillOptions should have comment or be unexported (revive)
  Warning: exported: exported method FrameFillOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported method ElementHandleWaitForElementStateOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewFrameGotoOptions should have comment or be unexported (revive)
  Warning: exported: exported method FrameGotoOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewFrameHoverOptions should have comment or be unexported (revive)
  Warning: exported: exported method FrameHoverOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewFrameInnerHTMLOptions should have comment or be unexported (revive)
  Warning: exported: exported method FrameInnerHTMLOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewFrameInnerTextOptions should have comment or be unexported (revive)
  Warning: exported: exported method FrameInnerTextOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewFrameInputValueOptions should have comment or be unexported (revive)
  Warning: exported: exported method FrameInputValueOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewFrameIsCheckedOptions should have comment or be unexported (revive)
  Warning: exported: exported method FrameIsCheckedOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewFrameIsDisabledOptions should have comment or be unexported (revive)
  Warning: exported: exported method FrameIsDisabledOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewFrameIsEditableOptions should have comment or be unexported (revive)
  Warning: exported: exported method FrameIsEditableOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewFrameIsEnabledOptions should have comment or be unexported (revive)
  Warning: exported: exported method FrameIsEnabledOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported method FrameIsHiddenOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported method FrameIsVisibleOptions.Parse should have comment or be unexported (revive)
  Warning: exported: exported function NewFramePressOptions should have comment or be unexported (revive)
  Error: directive `//nolint:goconst` is unused for linter "goconst" (nolintlint)
  Error: directive `//nolint:exhaustive` is unused for linter "exhaustive" (nolintlint)
  Error: directive `//nolint:exhaustive` is unused for linter "exhaustive" (nolintlint)
  Error: directive `//nolint:funlen,cyclop` is unused for linter "cyclop" (nolintlint)
  Error: directive `//nolint:funlen,cyclop` is unused for linter "cyclop" (nolintlint)
  Error: directive `//nolint: exhaustive` is unused for linter "exhaustive" (nolintlint)
  Error: directive `//nolint:funlen` is unused for linter "funlen" (nolintlint)
  Error: directive `// nolint: cyclop` should be written without leading space as `//nolint: cyclop` (nolintlint)
  Error: directive `//nolint: goconst` is unused for linter "goconst" (nolintlint)
  Error: directive `//nolint:funlen` is unused for linter "funlen" (nolintlint)
  Error: directive `//nolint:funlen,cyclop` is unused for linter "cyclop" (nolintlint)
  Error: directive `//nolint:cyclop` is unused for linter "cyclop" (nolintlint)
  Error: directive `//nolint:cyclop` is unused for linter "cyclop" (nolintlint)
  Error: directive `//nolint:funlen,cyclop` is unused for linter "cyclop" (nolintlint)
  Error: directive `// nolint:exhaustive` should be written without leading space as `//nolint:exhaustive` (nolintlint)
  Error: directive `//nolint:funlen,cyclop` is unused for linter "funlen" (nolintlint)
  Error: directive `//nolint:funlen,cyclop,gocognit` is unused for linter "funlen" (nolintlint)
  Error: directive `//nolint:funlen` is unused for linter "funlen" (nolintlint)
  Error: directive `//nolint:gocognit` is unused for linter "gocognit" (nolintlint)
  Error: directive `//nolint:forcetypeassert` is unused for linter "forcetypeassert" (nolintlint)
  Error: directive `//nolint:forcetypeassert` is unused for linter "forcetypeassert" (nolintlint)
  Error: directive `//nolint:cyclop` is unused for linter "cyclop" (nolintlint)
  Error: directive `//nolint:funlen` is unused for linter "funlen" (nolintlint)
  Error: directive `//nolint:funlen` is unused for linter "funlen" (nolintlint)
  Error: directive `//nolint:exhaustive` is unused for linter "exhaustive" (nolintlint)
  
  Error: issues found

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:

  1. Allow the use of our own .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.
  2. 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.
  3. Fix them now, but this would take time away from some other more pressing issues.

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?

@ankur22
Copy link
Collaborator

ankur22 commented May 3, 2024

@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:

  1. Only work with the lint workflow for now, as long as we can resolve the issue detailed in this comment.
  2. Amend test-go-versions so that it accepts arguments.

WDYT?

@inancgumus
Copy link
Member Author

@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:

  1. Only work with the lint workflow for now, as long as we can resolve the issue detailed in this Migrating to k6-core's linter #1292 (comment).
  2. 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.

@codebien
Copy link

codebien commented May 3, 2024

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. 👍

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.

@ankur22
Copy link
Collaborator

ankur22 commented May 3, 2024

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.

@ankur22 ankur22 changed the title Migrating with k6-core's linter Migrating to k6-core's linter May 8, 2024
@ankur22
Copy link
Collaborator

ankur22 commented May 20, 2024

  1. The plan is to copy/use the lint file from here and use it in this project, and add only-new-issues: true to that file so that only future changes will be linted.
  2. See if it's possible to amend test-go-versions so that it accepts arguments.

@ankur22 ankur22 removed their assignment May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci compatibility k6 core compatibility security
Projects
None yet
Development

No branches or pull requests

3 participants