| -@gerrit:opendev.org- Zuul merged on behalf of James E. Blair https://matrix.to/#/@jim:acmegating.com: [zuul/zuul-sphinx] 817650: Add :zuul:path support https://review.opendev.org/c/zuul/zuul-sphinx/+/817650 | 00:12 | |
| @jim:acmegating.com | zuul-maint: how does this look for a zuul-sphinx release? commit a8465c7cc1db65f86665e0446f9f4e07a441dc05 (HEAD -> master, tag: 0.6.0, origin/master, origin/HEAD, refs/changes/50/817650/3) | 00:14 |
|---|---|---|
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 809300: WIP: Add a zookeeper map to developer docs https://review.opendev.org/c/zuul/zuul/+/809300 | 00:16 | |
| @fungicide:matrix.org | > <@harrymichal:matrix.org> > martymichal: (you may know this, but just to be clear: zuul manages metadata about the artifact, like the storage location; it's up to jobs and roles to implement the actual storage and fetching of it; the zuul-jobs repo has some roles to help with that) | 00:36 |
| > | ||
| > Didn't know that. So, I'll have to check those out before I start writing the config for the artifacts. Before I do so, does this require the project to acquire some actual storage space? | ||
| not necessarily, for example in our case we save artifacts to the same location as build logs. i guess it depends on what you're storing and what you already have available, but zuul does not itself include any log or artifact storage service directly, just ansible roles for using external storage services | ||
| @harrymichal:matrix.org | > not necessarily, for example in our case we save artifacts to the same location as build logs. i guess it depends on what you're storing and what you already have available, but zuul does not itself include any log or artifact storage service directly, just ansible roles for using external storage services | 01:20 |
| Ah, we could probably use that, too. Thanks! Wrapping my head around Zuul is quite challenging after being used to hosted systems like GitHub Actions or GitLab CI. | ||
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 817668: Only update ZKObject values in ZK if necessary https://review.opendev.org/c/zuul/zuul/+/817668 | 03:13 | |
| @jim:acmegating.com | tobiash, swest, Clark: ^ watching the opendev logs, i think that may make a significant difference in efficiency | 03:14 |
| -@gerrit:opendev.org- Zuul merged on behalf of Felix Edel: | 04:37 | |
| - [zuul/zuul] 814996: Make the ConfigLoader work independently of the Scheduler https://review.opendev.org/c/zuul/zuul/+/814996 | ||
| - [zuul/zuul] 816361: Load system config and tenant layouts in zuul-web https://review.opendev.org/c/zuul/zuul/+/816361 | ||
| -@gerrit:opendev.org- Zuul merged on behalf of Felix Edel: [zuul/zuul] 816362: Implement job freezing API in zuul-web https://review.opendev.org/c/zuul/zuul/+/816362 | 04:37 | |
| @cjacobs3:matrix.org | How does zuul deal with merge commits on Gerrit and the job.files attribute? | 08:49 |
| I see situations where zuul is not triggering jobs based on changed files in a merge commit on gerrit leading to commits being merged without proper testing. | ||
| Example: | ||
| branch1 is being merged with master, a commit "merge branch1" is pushed to the master branch staging area on Gerrit. Branch1 contains many changes to files which we use the job.files attribute to trigger jobs on, but none of them are triggered. | ||
| I then test by manually checking out the files in branch1 and push them to the gerrit master staging area, creating a normal commit, not a merge commit. In this case zuul triggers jobs just as expected. | ||
| Why this discrepancy? Why is zuul not seeing the changed files in our merge commit and triggering accordingly? | ||
| -@gerrit:opendev.org- Zuul merged on behalf of Felix Edel: [zuul/zuul] 816514: Handle management events directly in zuul-web https://review.opendev.org/c/zuul/zuul/+/816514 | 09:11 | |
| @ekapoun1:matrix.org | cjacobs3: Is this related? http://lists.zuul-ci.org/pipermail/zuul-discuss/2018-November/000616.html | 09:44 |
| Albin Vass Could anyone from the Zuul-project chime in on this? Basically we're having an issue where Zuul will not run our check-jobs (each with a defined "files:" property) on merge commits from a branch into master on Gerrit. Is this a limitation of Zuul or a misconfiguration on our side? | ||
| -@gerrit:opendev.org- Felix Edel proposed on behalf of Simon Westphahl: | 09:55 | |
| - [zuul/zuul] 815278: DNM: execute tests with two schedulers https://review.opendev.org/c/zuul/zuul/+/815278 | ||
| - [zuul/zuul] 815787: Refresh pipelines in tests when settled https://review.opendev.org/c/zuul/zuul/+/815787 | ||
| -@gerrit:opendev.org- Felix Edel proposed: [zuul/zuul] 817724: WIP Share FakeRepository objects across FakeGithubConnections https://review.opendev.org/c/zuul/zuul/+/817724 | 09:55 | |
| @avass:vassast.org | cjacobs3: ekapoun1 it looks like the github and gerrit drivers use their repsective apis to figure which files changed. so my guess is that a github returns the list of files changed in the PR while gerrit doesn't for a merge commit | 09:55 |
| @avass:vassast.org | https://opendev.org/zuul/zuul/src/branch/master/zuul/driver/github/githubconnection.py#L1499 | 09:55 |
| @avass:vassast.org | https://opendev.org/zuul/zuul/src/branch/master/zuul/driver/gerrit/gerritconnection.py#L1315 | 09:56 |
| -@gerrit:opendev.org- Zuul merged on behalf of Felix Edel: [zuul/zuul] 816783: Implement autohold endpoints directly in zuul-web https://review.opendev.org/c/zuul/zuul/+/816783 | 10:16 | |
| @danygankoff:matrix.org | Hello everyone, | 10:24 |
| I had written an email to zuul-discuss@lists.zuul-ci.org on November 1st, but it is still in moderation. Could I ask my question here? | ||
| @avass:vassast.org | danygankoff: yeah shoot. corvus you may wanna take a look at that ^ :) | 10:25 |
| -@gerrit:opendev.org- Zuul merged on behalf of Simon Westphahl: | 10:27 | |
| - [zuul/zuul] 817003: Store pipeline summary for zuul-web in Zookeeper https://review.opendev.org/c/zuul/zuul/+/817003 | ||
| - [zuul/zuul] 817004: Use pipeline summary from Zookeeper in zuul-web https://review.opendev.org/c/zuul/zuul/+/817004 | ||
| -@gerrit:opendev.org- Zuul merged on behalf of Simon Westphahl: | 10:28 | |
| - [zuul/zuul] 817157: Use abide for listing pipelines in zuul-web https://review.opendev.org/c/zuul/zuul/+/817157 | ||
| - [zuul/zuul] 817158: Use abide for getting public keys in zuul-web https://review.opendev.org/c/zuul/zuul/+/817158 | ||
| @danygankoff:matrix.org | I've got a problem while upgrading Zuul from 4.9 to 4.10. | 10:30 |
| Our node list consists of static machines only. We monitor their availability via the Nodes web page (so the REST API). In 4.10, that list doesn't include the ones that are not in use. However, calling `nodepool list` returns the expected list with ready nodes in it. | ||
| I've found the nodes are filtered here [2]. The `node.user_data` attribute of the ready nodes is `None`, and, as I understood this commit message [3], this is the correct behavior. | ||
| So, is there a way to list the ready nodes too without the direct CLI call? | ||
| [1] https://zuul-ci.org/docs/zuul/reference/releasenotes.html#upgrade-notes | ||
| [2] https://review.opendev.org/plugins/gitiles/zuul/zuul/+/refs/heads/master/zuul/web/__init__.py#933 | ||
| [3] https://review.opendev.org/plugins/gitiles/zuul/zuul/+/aee6ef6f7f93c3c1dccd0576165d71ac1eecd13e%5E%21/zuul/web/__init__.py | ||
| -@gerrit:opendev.org- Zuul merged on behalf of Simon Westphahl: | 10:33 | |
| - [zuul/zuul] 817164: Use abide for getting project SSH keys in zuul-web https://review.opendev.org/c/zuul/zuul/+/817164 | ||
| - [zuul/zuul] 817171: Check authentication directly in zuul-web https://review.opendev.org/c/zuul/zuul/+/817171 | ||
| -@gerrit:opendev.org- Zuul merged on behalf of Simon Westphahl: | 10:34 | |
| - [zuul/zuul] 817172: Create list of admin tenants directly in zuul-web https://review.opendev.org/c/zuul/zuul/+/817172 | ||
| - [zuul/zuul] 817174: Use abide to get (dis-)allowed labels in zuul-web https://review.opendev.org/c/zuul/zuul/+/817174 | ||
| -@gerrit:opendev.org- Zuul merged on behalf of Simon Westphahl: [zuul/zuul] 817177: Create list of connections directly in zuul-web https://review.opendev.org/c/zuul/zuul/+/817177 | 10:34 | |
| -@gerrit:opendev.org- Zuul merged on behalf of Simon Westphahl: [zuul/zuul] 817193: Use config errors directly from layout in zuul-web https://review.opendev.org/c/zuul/zuul/+/817193 | 10:36 | |
| -@gerrit:opendev.org- Zuul merged on behalf of Felix Edel: [zuul/zuul] 817159: Use abide to look up jobs directly in zuul-web https://review.opendev.org/c/zuul/zuul/+/817159 | 10:36 | |
| -@gerrit:opendev.org- Zuul merged on behalf of Felix Edel: [zuul/zuul] 817160: Use abide to look up projects directly in zuul-web https://review.opendev.org/c/zuul/zuul/+/817160 | 10:36 | |
| @tobias.henkel:matrix.org | danygankoff: you get all nodes with a call to the rest api of nodepool directly | 11:41 |
| @tobias.henkel:matrix.org | it's the /node-list endpoint | 11:42 |
| @tobias.henkel:matrix.org | by default that returns a text table, but if you set the `accept: application/json` header it will return json | 11:44 |
| @tobias.henkel:matrix.org | corvus: I've noticed that on this change pipeline supercedes didn't seem to work, after the recheck both check and gate reported | 12:04 |
| @tobias.henkel:matrix.org | https://review.opendev.org/c/zuul/zuul/+/817196/8 | 12:04 |
| @cjacobs3:matrix.org | Albin Vass: ekapoun1 | 12:42 |
| According to this link Gerrit by default send the file list from the "auto-merge" section on Gerrit. That section (at least for us) won't show any changes. All file changes lie under "parent1", with parent1 meaning changes compared to the branch that we are currently merging to. | ||
| https://groups.google.com/g/repo-discuss/c/oDVJQv7mr38 | ||
| seems possible to control but i don't know what other implications it might have | ||
| @danygankoff:matrix.org | > <@tobias.henkel:matrix.org> it's the /node-list endpoint | 13:44 |
| There's no `/node-list` endpoint. Did you mean the `/nodes` one? So, the problem is that there are no `ready` state nodes in the endpoint's output since the 4.10 version. But they are printed by the direct CLI call. | ||
| The Quick-Start Tutorial's `docker-compose up` step is enough to reproduce my problem. `/api/tenant/example-tenant/nodes` returns an empty list, but `docker-compose exec launcher nodepool list` prints one ready-state node. | ||
| @tobias.henkel:matrix.org | danygankoff: nodepool has that endpoint, not zuul | 13:49 |
| @tobias.henkel:matrix.org | https://zuul-ci.org/docs/nodepool/operation.html#get--node-list | 13:50 |
| @tobias.henkel:matrix.org | (I mean as an alternative to the filtered nodes provided by zuul) | 13:51 |
| @danygankoff:matrix.org | > <@tobias.henkel:matrix.org> (I mean as an alternative to the filtered nodes provided by zuul) | 13:58 |
| Thanks. As I understood, since 4.10 Nodepool and Zuul produced lists are not same. | ||
| @jim:acmegating.com | cjacobs3, ekapoun1: what version of zuul are you using? there were significant changes in 4.10 to deal with merge commits in gerrit. i'd recommend that if you aren't running it. | 14:02 |
| @jim:acmegating.com | danygankoff: if you subscribe to the list, subsequent emails won't be moderated, and as a bonus, you'll receive the replies :) | 14:03 |
| @ekapoun1:matrix.org | corvus: The Zuul Status page gives us: | 14:03 |
| Zuul version: 4.9.0.0acme4 ac697e06 | ||
| @ekapoun1:matrix.org | Seems like we're a bit behind :) Let's see if/when an upgrade is coming on our side | 14:04 |
| @jim:acmegating.com | ekapoun1: oh are you one of my clients? :) there is a 4.10.4-acme1 that should have that fix. | 14:05 |
| @ekapoun1:matrix.org | corvus: It's possible that we work/collaborate with the same people (Johannes Foufas). Johannes recommended us to contact this chat for some questions we have, hope that's alright. | 14:08 |
| @jim:acmegating.com | ekapoun1: any zuul questions are welcome here, though be aware people who might answer them are volunteering their time. :) i do have private support channels for clients, which might be best if it's related to acme zuul, but your question is generally relevant, so certainly not off-topic. | 14:15 |
| @avass:vassast.org | Heh, interesting :) | 14:27 |
| -@gerrit:opendev.org- Zuul merged on behalf of Felix Edel: | 16:07 | |
| - [zuul/zuul] 817196: Use abide for listing tenants in zuul-web https://review.opendev.org/c/zuul/zuul/+/817196 | ||
| - [zuul/zuul] 817228: Remove gearman from zuul-web https://review.opendev.org/c/zuul/zuul/+/817228 | ||
| -@gerrit:opendev.org- Zuul merged on behalf of Felix Edel: | 16:07 | |
| - [zuul/zuul] 817354: Align common parts in zuul-web API methods https://review.opendev.org/c/zuul/zuul/+/817354 | ||
| - [zuul/zuul] 817363: Don't let zuul-web wait for management events to complete https://review.opendev.org/c/zuul/zuul/+/817363 | ||
| @clarkb:matrix.org | corvus: have you had a chance to look at the comments in https://review.opendev.org/c/zuul/zuul/+/817652/1/zuul/scheduler.py (and the greater change) ? | 17:23 |
| @clarkb:matrix.org | basically where I ended up when writing that change is that the one test doesn't work because we load zuul tenant configs pre validation and you get a completely different error (and less human friendly error) than would be reported by the validator | 17:24 |
| @jim:acmegating.com | Clark: maybe add a voluptuous call on data in ConfigLoader.readConfig ? | 17:30 |
| @clarkb:matrix.org | corvus: ya I guess we can just force it to be called earlier and then maybe drop the call from TenantParser.fromYaml()? | 17:34 |
| @jim:acmegating.com | yeah, that sounds reasonable | 17:36 |
| @clarkb:matrix.org | looking at scheduler's validateTenants there maybe some chicken and egg there. I'll have to look at it more closely after some breakfast | 17:38 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 817798: Remove QueueItem.debug method https://review.opendev.org/c/zuul/zuul/+/817798 | 17:50 | |
| @clarkb:matrix.org | corvus: https://review.opendev.org/c/zuul/zuul/+/817798 and https://review.opendev.org/c/zuul/zuul/+/817668 are both intended to be performance improvements? | 18:06 |
| @jim:acmegating.com | Clark: yes -- 668 is uncovering a bunch of things that just happen to work because we frequently write out data to zk; so if we make it more efficient, it breaks. 798 is one of those and i thought that was a good change to do standalone for easier review. so it's a pre-req for the other. i've run tests on it locally, it's ready to go. i'm currently restacking 668 on top of it locally and fixing it up. | 18:08 |
| @tobias.henkel:matrix.org | I was already wondering why 668 broke | 18:23 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 817668: Only update ZKObject values in ZK if necessary https://review.opendev.org/c/zuul/zuul/+/817668 | 18:37 | |
| @jim:acmegating.com | okay i think that should work now. i'm wondering if it's worth making some kind of immutable dict/list object that only allows mutation when a context is active. that's our weak spot where we could make changes without knowing we need to write to zk. | 18:40 |
| @tobias.henkel:matrix.org | corvus: 668 lgtm except a print leftover in the test case | 18:46 |
| @jim:acmegating.com | doh | 18:46 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 817668: Only update ZKObject values in ZK if necessary https://review.opendev.org/c/zuul/zuul/+/817668 | 18:47 | |
| -@gerrit:opendev.org- Clark Boylan proposed: [zuul/zuul] 817652: Prevent duplicate config file entries https://review.opendev.org/c/zuul/zuul/+/817652 | 18:58 | |
| @clarkb:matrix.org | corvus: ^ readConfig updated to check the vs schema. I've not removed the other vs schema check which might be a bug (left a todo about it) | 18:59 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed on behalf of Matthieu Huin https://matrix.to/#/@mhuin:matrix.org: [zuul/zuul] 817388: Add testing for tenant authorizations override https://review.opendev.org/c/zuul/zuul/+/817388 | 19:00 | |
| @jim:acmegating.com | Clark: 2 questions: 1) don't you need to do it before line 2297? and 2) is there any reason to limit it to specific tenants? if the config file syntax is wrong, it's wrong... | 19:03 |
| @clarkb:matrix.org | corvus: for 2) the reason is the validate tenants command which can take a subset of tenants. Otherwise everything will be validated. | 19:04 |
| @clarkb:matrix.org | for 1) which file? | 19:04 |
| @jim:acmegating.com | configloader | 19:05 |
| @clarkb:matrix.org | ah ok for 1) the issue is in loadTPCs because it is actually using the data. I think as long as we're checking the schema before we loadTPCs we're ok | 19:05 |
| @clarkb:matrix.org | we load TPCs before calling loadTenant which is what currently validates the tenant configs. This means if your config is not good then loadTPCs can raise and you get a less nice error message than what vs will produce | 19:06 |
| @jim:acmegating.com | okay, i guess UnparsedAbideConfig.extend does sufficient validation of what it manipulates | 19:06 |
| @clarkb:matrix.org | Ya I think as long as it is valid yaml it will get by? | 19:14 |
| @jim:acmegating.com | Clark: for 2) there are 2 kinds of validation -- the schema validation you're concerned with, and the actual config validation which requires mergers and cat jobs and everything. That's the main reason validateConfig takes the tenant list argument. I figure if we're moving schema validation earlier, we can probably just do the whole file since it's cheap. but i don't object to the way you've written the change -- it matches current behavior. | 19:15 |
| @clarkb:matrix.org | And then we need to make the next step of ensuring the yaml conforms to our schema | 19:15 |
| @jim:acmegating.com | Clark: lgtm; i think we probably can remove the other validation; i'm pretty sure with your update we can't get an invalid config into memory/zk. i guess the only thing i can think of there is maybe we could put a config that's valid on one scheduler into zk which isn't valid on another. having the extra validation would stop a reconfiguration early, whereas without it, we might fail halfway through. | 19:19 |
| @jim:acmegating.com | maybe it's worth keeping for that purpose? if you think so, maybe just change that TODO note to say that. otherwise, i'd say remove it. | 19:20 |
| @jim:acmegating.com | (schema validation is pretty cheap, so i think i'm like 51% on the side of keep it) | 19:21 |
| @clarkb:matrix.org | that actually seems useful particularly if people start upgrading multiple schedulers with big jumps that might expect slgihtyl different configs | 19:24 |
| @clarkb:matrix.org | having the more explicit error message might give them a clue they need to do a full restart? | 19:25 |
| @clarkb:matrix.org | I'll update | 19:25 |
| @jim:acmegating.com | ++ | 19:25 |
| @jim:acmegating.com | Clark: if you have a minute to look at https://review.opendev.org/817668 today, that'd be great; i think that's a really good one for me to try running in opendev this evening/weekend :) | 19:25 |
| @clarkb:matrix.org | I should. Sorry I'm catching up on a bunch of stuff I ignored while sick and updating these changes. I'll be able to get back to zoom shortly I hope | 19:27 |
| @erbarr:matrix.org | does a job's cleanup-run execute all the time or only in aborts? | 19:33 |
| @jim:acmegating.com | erbarr: always: https://zuul-ci.org/docs/zuul/reference/job_def.html#attr-job.cleanup-run | 19:33 |
| @jim:acmegating.com | (to the best of zuul's ability, of course) | 19:34 |
| @clarkb:matrix.org | * I should. Sorry I'm catching up on a bunch of stuff I ignored while sick and updating these changes. I'll be able to get back to zuul shortly I hope | 19:34 |
| @erbarr:matrix.org | mmnn, i must have something wrong then, it can be inherited from a base job, right? | 19:34 |
| @jim:acmegating.com | erbarr: yes; child jobs append to the end (just like post-run) | 19:36 |
| @jim:acmegating.com | correction, they prepend, just like post-run | 19:37 |
| -@gerrit:opendev.org- Clark Boylan proposed: [zuul/zuul] 817652: Prevent duplicate config file entries https://review.opendev.org/c/zuul/zuul/+/817652 | 19:39 | |
| @clarkb:matrix.org | corvus: ^ updated | 19:39 |
| @erbarr:matrix.org | okay thanks | 19:40 |
| @spamaps:spamaps.ems.host | 18 months working in TypeScript has really made me wish for type annotations in the Zuul codebase. | 19:43 |
| @spamaps:spamaps.ems.host | corvus: Working on pods-in-one-namespace and wondering if allowed-labels is sufficient to ensure that only authorized tenants can use a particular label. That should be sufficient to provide pre-configured namespaces securely. | 19:58 |
| @fungicide:matrix.org | spamaps: earlier attempts to annotate the codebase didn't go so well, though it's possible using external annotations a la typeshed style wouldn't be so intrusive | 20:06 |
| @fungicide:matrix.org | at least that way they'd be in separate files (possibly even a separate repo), so wouldn't clutter up the code | 20:07 |
| @spamaps:spamaps.ems.host | > <@fungicide:matrix.org> at least that way they'd be in separate files (possibly even a separate repo), so wouldn't clutter up the code | 20:09 |
| Frankly I think what you call clutter, I call clarity. It's really just a difference in style. External types are quite a bit less helpful. | ||
| @fungicide:matrix.org | though also not shooting for 100% annotation and expecting all teh duck typing to be refactored out of the software might be attainable | 20:09 |
| @jim:acmegating.com | > <@spamaps:spamaps.ems.host> corvus: Working on pods-in-one-namespace and wondering if allowed-labels is sufficient to ensure that only authorized tenants can use a particular label. That should be sufficient to provide pre-configured namespaces securely. | 20:09 |
| I think so. | ||
| @spamaps:spamaps.ems.host | > <@fungicide:matrix.org> though also not shooting for 100% annotation and expecting all teh duck typing to be refactored out of the software might be attainable | 20:10 |
| Any is a useful type. | ||
| @spamaps:spamaps.ems.host | There are still a lot of big corner cases in the python type annotation scheme. Nobody thinks it is fully expressive. But I've switched to the darkside.. I use vs:code now.. and not having auto-complete for properties and methods is really cramping my style. | 20:11 |
| @fungicide:matrix.org | spamaps: well, the up-side to external annotations is that they can be maintained separately by the people who want annotations, and not get in the way of people who would rather not see them | 20:11 |
| @jim:acmegating.com | We've been removing them, please don't add them. | 20:11 |
| @spamaps:spamaps.ems.host | > <@fungicide:matrix.org> spamaps: well, the up-side to external annotations is that they can be maintained separately by the people who want annotations, and not get in the way of people who would rather not see them | 20:12 |
| That's a downside for people who want the code to be more readable. :) | ||
| @spamaps:spamaps.ems.host | I won't add any. But I fundamentally think you have shut out a whole new generation of developers who benefit from this flow. | 20:12 |
| @fungicide:matrix.org | this is basically the same in the python stdlib. annotations are kept external to the code so that people who want them can have them | 20:12 |
| @spamaps:spamaps.ems.host | > <@fungicide:matrix.org> this is basically the same in the python stdlib. annotations are kept external to the code so that people who want them can have them | 20:14 |
| I think that's a bit of a worst-case solution. Either commit to them, or don't have them. | ||
| @fungicide:matrix.org | spamaps: "readable" is a relative concept where annotations are concerned, it seems | 20:14 |
| @spamaps:spamaps.ems.host | Now they're 2nd-class citizens and that's a major bummer. | 20:14 |
| @jim:acmegating.com | Sorry you feel that way. It's not the intent. Some people feel the exact opposite so let's try to be generous and not assume anyone wants to shut anyone out. | 20:15 |
| @spamaps:spamaps.ems.host | > <@fungicide:matrix.org> spamaps: "readable" is a relative concept where annotations are concerned, it seems | 20:15 |
| Correct, 100% agree, pick a style and stick to it. I do not like this choice but I do not need it to be my way to be effective. | ||
| @fungicide:matrix.org | if you need a separate hint-aware ide in order to make the code readable, and it becomes considerably less readable for people using a plain text editor, then that seems like a worst-case solution to me | 20:15 |
| @spamaps:spamaps.ems.host | > <@jim:acmegating.com> Sorry you feel that way. It's not the intent. Some people feel the exact opposite so let's try to be generous and not assume anyone wants to shut anyone out. | 20:16 |
| It's an unintended consequence of the style choice. I of course know nobody wants to shut new folks out. | ||
| @jim:acmegating.com | Or old folks. | 20:16 |
| @fungicide:matrix.org | particularly if you need a proprietary closed-source ide like vs-code to be able to effectively contribute, that would be a significant step backwards | 20:17 |
| @spamaps:spamaps.ems.host | > <@fungicide:matrix.org> if you need a separate hint-aware ide in order to make the code readable, and it becomes considerably less readable for people using a plain text editor, then that seems like a worst-case solution to me | 20:18 |
| Sorry I was not clear. I think type annotations make all of the code more readable on face value. Knowing what a value will be helps me read the code where it is, not have to dig through where it's called, or hope that the method signature is accurate. The IDE capitalizes on this by reading the code for you, and allowing you to forget those things until they are relevant. | ||
| @spamaps:spamaps.ems.host | You see noise, I see signal. Neither of us are always wrong or right. :) | 20:18 |
| @spamaps:spamaps.ems.host | vs:code is open source. | 20:18 |
| @jim:acmegating.com | I hear that and understand it, and for me it's exactly opposite, so until types in python get better it's Kobayashi Maru. | 20:19 |
| @spamaps:spamaps.ems.host | TBH, I choose Rust for all new coding projects precisely because Python's type system is so half-baked. It leaves the door open to this problem. I'm sad for that. TypeScript is no panacea, but I like that they named the language something else entirely, so it's clear "This is what you're reading." | 20:20 |
| @fungicide:matrix.org | last i looked at it, the "vscodium" open source fork microsoft publishes isn't actually buildable | 20:20 |
| @spamaps:spamaps.ems.host | > <@fungicide:matrix.org> last i looked at it, the "vscodium" open source fork microsoft publishes isn't actually buildable | 20:21 |
| Damn that's a shame. :-P | ||
| @spamaps:spamaps.ems.host | Also btw, I started this journey with neovim and auto-complete so vs:code is just a progression beyond that which I've taken to align with my coroorkers. :) | 20:22 |
| @spamaps:spamaps.ems.host | * Also btw, I started this journey with neovim and auto-complete so vs:code is just a progression beyond that which I've taken to align with my coworkers. :) | 20:22 |
| @jim:acmegating.com | I like types in rust or even TS better than python. And I want to like types in python. | 20:22 |
| @jim:acmegating.com | > <@spamaps:spamaps.ems.host> Also btw, I started this journey with neovim and auto-complete so vs:code is just a progression beyond that which I've taken to align with my coworkers. :) | 20:23 |
| You have more than one set of coworkers. :) | ||
| @spamaps:spamaps.ems.host | Y'all are only recently back in that cohort. | 20:23 |
| @spamaps:spamaps.ems.host | And only in my 20% hack time at the moment. :) | 20:23 |
| @jim:acmegating.com | Ouch. | 20:23 |
| @spamaps:spamaps.ems.host | And to be specific, what I don't like is trying to dig three layers of the callstack back to figure out what exact object "self.k8s_client" is.. and then finding that not only is it not type annotated, it's auto-generating calls in runtime (ala openstack clients) and that means nobody will ever know what the types are. I really dislike auto-generated API clients. | 20:24 |
| @fungicide:matrix.org | spamaps: good news, looks like the situation with vscodium has improved in the last few months now. they've got cross-platform build scripts (and even a document of sorts) included. it relies on electron though, so getting it into debian would still be blocked on that for now: https://wiki.debian.org/Javascript/Nodejs/Tasks/electron | 20:36 |
| -@gerrit:opendev.org- Zuul merged on behalf of Matthieu Huin https://matrix.to/#/@mhuin:matrix.org: [zuul/zuul] 817388: Add testing for tenant authorizations override https://review.opendev.org/c/zuul/zuul/+/817388 | 20:52 | |
| @spamaps:spamaps.ems.host | Ah I see another reason type=pod works differently. The cleanup is much .. cleaner.. if you can just obliterate the whole namespace instead of just trying to find the specific pod that was created. | 20:57 |
| @spamaps:spamaps.ems.host | It's pretty interesting that cleanupNode only ever gets the external_id ... not the node object where it could discern things about how it was created. | 20:59 |
| @clarkb:matrix.org | corvus: just finishing up a few more small tasks then will get to that zkobject optimization change | 21:03 |
| @jim:acmegating.com | pushed zuul-sphinx 0.6.0 | 21:20 |
| @clarkb:matrix.org | corvus: nit/question on https://review.opendev.org/c/zuul/zuul/+/817668 Basically I'm wondering if we need to use the NULL object check at all if we instead always force callers of _save() to supply the data? | 21:25 |
| @clarkb:matrix.org | I guess I'm not seeing the need for that given we control all the call points and it makes the code a bit more complicated? | 21:25 |
| @clarkb:matrix.org | I think the only reason for it was to not update the _save call in new? But I think we can move the trySerialize out of there instead? I dunno its probably not a big deal, but seems like we're doing a lot of busy work for one case rather than updating the one case instead | 21:29 |
| @jim:acmegating.com | Clark: yeah, i think new is the only case | 21:32 |
| @jim:acmegating.com | i can update it with that | 21:32 |
| @clarkb:matrix.org | I think that is my preference simply for consistency sake. Thanks | 21:36 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 817668: Only update ZKObject values in ZK if necessary https://review.opendev.org/c/zuul/zuul/+/817668 | 21:36 | |
| @jim:acmegating.com | Clark, tobiash: ^ | 21:37 |
| @clarkb:matrix.org | +2 | 21:38 |
| -@gerrit:opendev.org- Zuul merged on behalf of James E. Blair https://matrix.to/#/@jim:acmegating.com: [zuul/zuul] 817798: Remove QueueItem.debug method https://review.opendev.org/c/zuul/zuul/+/817798 | 21:43 | |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 817824: Conditionally remove nodeset info https://review.opendev.org/c/zuul/zuul/+/817824 | 22:20 | |
| @jim:acmegating.com | Clark, fungi: ^ i think that's what we're seeing in opendev right now for 805391 | 22:20 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 817825: Only cancel jobs with incomplete builds https://review.opendev.org/c/zuul/zuul/+/817825 | 22:23 | |
| @jim:acmegating.com | that ^ is a followup that might make things more efficient. i don't know if it's a good idea/going to work, but thought i'd throw it at the test suite and see. | 22:24 |
| @clarkb:matrix.org | The test suite seems unhappy with my vs schema checking change. I probably wont' get to it today, but will try to pick that up soon | 22:24 |
| @clarkb:matrix.org | I feel better today, but it feels like the cold is catching back up with me | 22:25 |
| @jim:acmegating.com | tobiash: if you're still around and have a quick minute for https://review.opendev.org/817824 that'd be great; it's not critical, it's just annoying | 22:25 |
| @jim:acmegating.com | Clark: there's at least 3 more new() overrides that are going to need that serialization change | 22:59 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 817668: Only update ZKObject values in ZK if necessary https://review.opendev.org/c/zuul/zuul/+/817668 | 23:01 | |
| @clarkb:matrix.org | Oh sorry I should've checked for other uses of new | 23:05 |
| @jim:acmegating.com | i'm running tests locally on that to try to make sure we can get it merged today | 23:05 |
| @jim:acmegating.com | no local failures; i went ahead and reapproved it | 23:17 |
| @clarkb:matrix.org | corvus: just occurred to me. Do we need to serialize with sorted keys to make them consistent? We are comparing strings right? | 23:23 |
| @clarkb:matrix.org | Maybe we already do that? | 23:24 |
| @jim:acmegating.com | hrm, i don't think we do... we might have to touch a bunch of serialize() methods for that | 23:24 |
| @jim:acmegating.com | Clark: i think we're okay because it's python3.6+ though? | 23:26 |
| @jim:acmegating.com | i know tristanC recently suggested a sorted keys arg for json for explcitness, and i agreed. and maybe it's worth doing the same thing here for the same reason, even if technically not necessary right now... | 23:27 |
| @clarkb:matrix.org | Hrm ya 3.6 sorts keys by default but does the json module? We can probably check it really quick? | 23:28 |
| @clarkb:matrix.org | I want to say it is pretty consistent when serializing based on my experience with the Gerrit json and doing account cleanups | 23:31 |
| @clarkb:matrix.org | I can python -m json.tool that and get consistent results. Buy maybe it is just preserving the order from Gerrit and that is consistent. I'll have to do some testing | 23:31 |
| @clarkb:matrix.org | corvus: I think this can be problematic if the order things are added to the dict changes | 23:37 |
| @tristanc_:matrix.org | Clark: json key order should not be guaranteed according to the specification. And if we are going to use json serialization to compare data, then i think it is required to sort the keys. | 23:37 |
| @clarkb:matrix.org | json.dumps() seems to respect dict entry order | 23:37 |
| @clarkb:matrix.org | tristanC: yes that is why I brought it up, I'm just trying to figure out how important it is for this particular change. | 23:38 |
| @clarkb:matrix.org | ``` | 23:39 |
| >>> d1 = {1:2, 3:4, 'a':'b', 'c':'z', 'd':'e'} | ||
| >>> d2 = {1:2, 3:4, 'a':'b', 'c':'z', 'd':'e'} | ||
| >>> import json | ||
| >>> json.dumps(d1) | ||
| '{"1": 2, "3": 4, "a": "b", "c": "z", "d": "e"}' | ||
| >>> json.dumps(d2) | ||
| '{"1": 2, "3": 4, "a": "b", "c": "z", "d": "e"}' | ||
| >>> d3 = {'a':'b', 3:4, 'd':'e', 'c':'z', 1:2} | ||
| >>> json.dumps(d3) | ||
| '{"a": "b", "3": 4, "d": "e", "c": "z", "1": 2}' | ||
| @tristanc_:matrix.org | Clark: then `d1 == d3` but `json.dumps(d1) != json.dumps(d3)` right? | 23:40 |
| @clarkb:matrix.org | tristanC: correct | 23:41 |
| @clarkb:matrix.org | I suspect this will mostly work in the zuul code base since we construct these objects in consistent orders. And updates to keys don't shift their order | 23:45 |
| @clarkb:matrix.org | However, I think we probably should do a followup that does ordered dumping? | 23:45 |
| @clarkb:matrix.org | Unrelated ti seems that tumbleweed has broken `view` by symlinking it to `vim` which is symlinked to `alts` which doesn't have an entry for `view` | 23:46 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 817827: Use sort_keys with json almost everywhere we write to ZK https://review.opendev.org/c/zuul/zuul/+/817827 | 23:47 | |
| @jim:acmegating.com | Clark, tristanC: ^ there's the followup. I agree it's not necessary, but is a good idea. an alternative would be to do a json dump/load cycle in zkobject so we're comparing a deserialized object. but maybe this is better + more efficient (the other would be only 2 lines of code, but wasteful) | 23:49 |
| @jim:acmegating.com | oh, i left something in there i didn't mean to; 1 sec | 23:49 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 817827: Use sort_keys with json almost everywhere we write to ZK https://review.opendev.org/c/zuul/zuul/+/817827 | 23:50 | |
| @clarkb:matrix.org | I left a comment about that then refreshed and it was fixed :) | 23:51 |
| @tristanc_:matrix.org | Clark: corvus well i'm not sure how can you tell this is not necessary, but i think it is worth adding protections for such issue. | 23:53 |
| @jim:acmegating.com | tristanC: because json dumps honors python dict key insertion order, and we always insert our keys in the same order in the serialize() methods. so for this specific use case, at this specific time, it's not necessary. but several of those things could change so it's good to be defensive. | 23:54 |
| @jim:acmegating.com | i want to merge that change, but i don't think i need to wait for it before restarting opendev to collect more real-world data since it should have the same effect | 23:55 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!