| mborzecki | morning | 06:08 |
|---|---|---|
| pstolowski | morning | 07:05 |
| mborzecki | pstolowski: hey, welcome back | 07:07 |
| mup | PR snapd#10235 opened: asserts: fix errors reported by linter <Created by mardy> <https://github.com/snapcore/snapd/pull/10235> | 07:20 |
| mborzecki | pstolowski: can you take a look at ^^ ? | 07:43 |
| mup | PR snapd#10232 closed: cmd/snap-bootstrap: Fix linter errors <Created by mardy> <Merged by bboozzoo> <https://github.com/snapcore/snapd/pull/10232> | 07:45 |
| mup | PR snapd#10233 closed: cmd/snap-repair: fix linter issues <Created by mardy> <Merged by bboozzoo> <https://github.com/snapcore/snapd/pull/10233> | 07:45 |
| mborzecki | duh, rpkg segfaults on `rpkg srpm` guess rawhide isn't as stable as they say | 08:32 |
| pstolowski | mhm | 08:48 |
| mborzecki | we should probably add fedora 34 to our spread images now | 08:54 |
| pstolowski | mborzecki: do you need any reviews? | 08:55 |
| mborzecki | pstolowski: you can try https://github.com/snapcore/snapd/pull/10082 and give some feedback about the proposed config | 08:56 |
| mup | PR #10082: github: try out golangci-lint <Needs Samuele review> <Simple 😃> <Skip spread> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/10082> | 08:56 |
| mborzecki | pstolowski: there are prebuilt binaries here: https://github.com/golangci/golangci-lint/releases | 08:58 |
| pstolowski | ok | 08:58 |
| mup | PR snapd#10236 opened: spread: add Fedora 34, leave a TODO about dropping Fedora 32 <Simple 😃> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/10236> | 09:16 |
| mardy | mborzecki: do you know anything about this? https://github.com/snapcore/snapd/blob/master/bootloader/lkenv/lkenv.go#L536 | 09:28 |
| mardy | it's unused, but it's so well commented, that I'm unsure whether I should just remove it | 09:28 |
| mborzecki | mardy: iirc lkenv is a bit special, best leave it and ping ian in the comments | 09:29 |
| mup | PR snapd#10237 opened: client: Fix linter errors <Created by mardy> <https://github.com/snapcore/snapd/pull/10237> | 09:46 |
| pstolowski | pedronis: hi, maybe you have opinion on https://github.com/snapcore/snapd/pull/10215 ? | 10:03 |
| mup | PR #10215: store/store_download.go: make snapd resilient on slow network <Created by fujimotos> <https://github.com/snapcore/snapd/pull/10215> | 10:03 |
| mvo | mardy: hey, great to see your PRs :) just looked at 10237 and it looks great. one suggestion for things that just touch things like unit tests or reorder imports, you could use the "skip spread" label here because the risk that any of this breaks the integration testsuite is low. thanks and keep it up! | 10:25 |
| mup | PR snapd#10231 closed: cmd/snap: Fix errors reported by linter <Created by mardy> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/10231> | 10:36 |
| mardy | so, the "daemon" package testing setup is made with many "export_XXX_test.go" files, which export the types needed by the corresponding "XXX_test.go" | 11:00 |
| mardy | the "testpackage" linter does not like that, and would like a single "test_export.go" | 11:00 |
| mardy | should I (1) delete all these "export_XXX_test.go" files and move their contents to "test_export.go"? | 11:01 |
| mardy | or (2) rename them to ""export_XXX.go", so that the linter won't complain, and import them in the test which uses them? | 11:01 |
| ijohnson | mardy for now, just ignore the daemon package, it is a bit special and has been refactored recently and may be subject to more refactoring by Samuele so best to chat with him (after the sprint) about what to do for the daemon package | 11:06 |
| mvo | mardy: 10237 is green and has two +1 which means you can merge it if you want :) congrats for this | 11:16 |
| mardy | ijohnson: ok | 11:23 |
| mardy | mvo: done, thanks | 11:25 |
| mvo | mardy: congrats :) | 11:25 |
| mardy | ijohnson: for the daemon package then I'll just make a PR for the other changes, excluding the test stuff | 11:25 |
| mup | PR snapd#10237 closed: client: Fix linter errors <Skip spread> <Created by mardy> <Merged by mardy> <https://github.com/snapcore/snapd/pull/10237> | 11:26 |
| mup | PR snapd#10238 opened: daemon: fix linter errors <Created by mardy> <https://github.com/snapcore/snapd/pull/10238> | 11:31 |
| pedronis | mardy: ijohnson: yes, I have refactoring pendings for that package, the testpackage linter here sounds a bit naive, or mabye it telling us not subtly that the daemon package is too big | 11:37 |
| ijohnson | yeah maybe we could just disable that specific check for the daemon package | 11:38 |
| pedronis | maybe we just did it wrong all this time (sorry I can't really spend a lot of attention on this atm) | 11:53 |
| ijohnson | yeah no problem 🙂 like I said we can wait til after the sprint to dive back into what the best thing to do is I think | 11:54 |
| mup | PR snapd#10177 closed: tests: use op.paths tools instead of dirs.sh helper - part 2 <Created by sergiocazzolato> <Merged by sergiocazzolato> <https://github.com/snapcore/snapd/pull/10177> | 12:06 |
| mup | PR snapd#10239 opened: tests: fix tests expecting cgroup v1/hybrid on openSUSE Tumbleweed <Simple 😃> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/10239> | 12:06 |
| pstolowski | hey ijohnson | 12:26 |
| ijohnson | hey pstolowski | 12:37 |
| mardy | more questions about this testpackage linter: interfaces/connection_test.go uses the Attrer class, and if I change the package in the file to "interfaces_test", then it's not available anymore | 12:51 |
| mardy | so I do "type Attrer interfaces.Attrer" in the test file, and now it complains that the Attrer type was already defined... :-/ | 12:51 |
| pedronis | you need a type alias type Attrer = interfaces.Attrer ? or is that something else | 12:52 |
| ijohnson | mardy: I think you just need to change `Attrer` to `interfaces.Attrer` since that's exported ? | 12:52 |
| ijohnson | mardy: this works for me : | 12:54 |
| ijohnson | https://pastebin.ubuntu.com/p/nwS4yxYxb9/ | 12:54 |
| mardy | ijohnson: indeed! Thanks! | 12:57 |
| mborzecki | cachio: hm i think the github actions behavior is different for external cotributors | 13:18 |
| mborzecki | cachio: in https://github.com/snapcore/snapd/pull/10215 the actions did not run, i had to click on a button to make them run | 13:19 |
| mup | PR #10215: store/store_download.go: make snapd resilient on slow network <Needs Samuele review> <Created by fujimotos> <https://github.com/snapcore/snapd/pull/10215> | 13:19 |
| ijohnson | bboozzoo that was a change upstream GitHub action just made | 13:20 |
| mborzecki | ijohnson: cachio: https://github.blog/changelog/2021-04-22-github-actions-maintainers-must-approve-first-time-contributor-workflow-runs/ this one probbaly | 13:20 |
| mborzecki | hm looks like it's for first time contributors only? | 13:21 |
| ijohnson | yeah | 13:21 |
| ijohnson | To prevent crypto currency abuse | 13:21 |
| ijohnson | AIUI | 13:21 |
| mup | PR snapd#10240 opened: interfaces: fix linter errors <Created by mardy> <https://github.com/snapcore/snapd/pull/10240> | 13:26 |
| mardy | now that I added the package name, I get this error: | 14:53 |
| mardy | overlord/hookstate/context_test.go:91:39: unknown field `task` in struct literal (typecheck) anotherContext := &hookstate.Context{task: s.task, state: s.task.State(), setup: s.setup} | 14:53 |
| ijohnson | mardy: probably you need to add a new test-only helper to export_test.go that is something like `func MakeContext(t *state.Task, setup *Setup) { return Context{task: t, state: st, setup: setup} }` | 14:57 |
| pstolowski | mardy: that's because Context's members are private (unexported) so cannot be set from another package | 14:57 |
| ijohnson | right what pstolowski said | 14:57 |
| pstolowski | mardy: yea, ijohnson 's suggestion is the way to solve it | 14:57 |
| mardy | ah-ah, thanks guys! | 14:59 |
| ijohnson | np | 14:59 |
| pedronis | ijohnson: mardy: this starts to get a bit into weeds from my pov | 15:22 |
| pedronis | I'm not sure I agree with the testpackage imposed goal | 15:22 |
| ijohnson | pedronis: do you mean about moving a _test.go file which is currently in pkg foo to pkg foo_test ? you don't agree with that goal ? | 15:24 |
| pedronis | ijohnson: I agree is good, I'm not is good as an absolute goal, it's a judgement call | 15:25 |
| pedronis | *I'm not sure | 15:25 |
| ijohnson | I mean as an improvement to the codebase I think it makes sense regardless of whether we want to use this linter or not long term | 15:25 |
| pedronis | ijohnson: as I said, taken dogmatically it doesn't make sense | 15:26 |
| ijohnson | in my view, test files that are not in pkg foo_test and are in pkg foo, are not idiomatic go and so we should try to change them, time and energy allowing, to make it easier for us to maintain our codebase as a whole | 15:27 |
| pedronis | ijohnson: even assuming it's really the fundamental goal, we starts to have choices how to do it, and each needs to be judged on its own | 15:29 |
| pedronis | ijohnson: there are ways to achieve this that can get very messy | 15:30 |
| mvo | ijohnson: let's park this for a bit, we have meeting for a while :) | 15:30 |
| ijohnson | sure, I agree that if a simple PR from mardy is leading to this much discussion it's probably not an appropriate papercut PR for him to work on, I don't think anyone expected addressing these linter suggestions to lead to this much discussion | 15:31 |
| ijohnson | anyways meeting now it seems | 15:31 |
| pedronis | ijohnson: well I didn't know there was a dogmatic test linter | 15:32 |
| pedronis | involved | 15:32 |
| ijohnson | pedronis: well in my experience most linters are quite dogmatic for better or worse 🙂 | 15:53 |
| pedronis | ijohnson: remember that I didn't even accept that PR yet, I didn't have time to look into it | 15:55 |
| pedronis | yes, I'm aware that linters tend to be | 15:56 |
| ijohnson | sorry I think we are debating about the debate now, but anyways I think we should just have @mardy skip over this one and work on addressing different feedback | 15:56 |
| mup | PR snapd#10240 closed: interfaces: fix linter errors <Created by mardy> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/10240> | 19:12 |
| mup | PR snapd#10236 closed: spread: add Fedora 34, leave a TODO about dropping Fedora 32 <Simple 😃> <Created by bboozzoo> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/10236> | 19:17 |
| mup | PR snapd#10238 closed: daemon: fix linter errors <Created by mardy> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/10238> | 19:22 |
| mup | PR snapd#10239 closed: tests: fix tests expecting cgroup v1/hybrid on openSUSE Tumbleweed <Simple 😃> <Created by bboozzoo> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/10239> | 19:22 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!