Add a filter for maintainers in the jobset-eval view #743
Conversation
Ma27
force-pushed
the
jobset-search-maintainer
branch
from
43ed0d7
to
c314c86
Compare
|
Are you still working on this? I guess you still need help on the database migration matter? How about the part with retrieving maintainer info? |
Yes.
Well, the question is much rather, how do we want to migrate? Should we still keep the Or do you have an even better idea? :)
I'm not even sure if that's possible tbh. Does anybody have anough experience with the C++ API to accomplish that? (or, as a dirty hack, one may fetch the key from |
|
@gilligan what do you think of my suggestions regarding the DB-migration? And who else should I also ping for a review? While I'm happy to continue working on this, I'd prefer to get some feedback on this first :) |
|
@Ma27 do you have a few hundred gigabytes of free space to try out the migration on the prod dataset? If so, I can get you a copy. |
will be a bit close, but should be possible :) But first of all we should agree on how to implement the migration (I posted two ideas in the original comment). |
|
Just for the record, I'm happy to continue working on this (including a rebase to master :p), but first of all, I'd like to get some feedback and to get the migration-topic resolved. |
Ma27
force-pushed
the
jobset-search-maintainer
branch
2 times, most recently
from
da4beb1
to
34a4848
Compare
|
@grahamc in case you have time, could you please take a look at my comments about the migration needed for this change? |
|
ping @grahamc how should we proceed with the migration question? |
There was a problem hiding this comment.
For me I like the idea of it. One point of caution is the github handle can change often. I notice you cover this already: nicely done.
For the migration, in my most humble opinion it would be really good to retain the historical data. I think we might not have chosen to if we wrote the code ourselves for hydra.nixos.org, but I think overall other users of Hydra would find it important to have the data preserved.
If you agree, to that end we could do this a bit more relaxed than #710, but still in two releases:
- Create the new tables and start writing data to them
- Use the old column for all of the queries
- Extend
hydra-backfill-idsto operate over thebuildstable, reading and writing data to the maintainers and pivot table. I think the historical values of this field are all emails in the format ofName <email@domain>and can just be written directly in to theemailfield.
In a second release, swap reads over to the new table and then drop the old column.
A different route we could also take would be a single-release, but has a trade-off that the API and website would show null data for maintainers until the backfiller completes:
- Create the new tables and start writing data to them
- Use the new table for all of the queries, just as you've done here
- Extend
hydra-backfill-idsto operate over thebuildstable, reading and writing data to the maintainers and pivot table. I think the historical values of this field are all emails in the format ofName <email@domain>and can just be written directly in to theemailfield. - At the end of
hydra-backfill-ids, drop the old column.
Do you have any opinions or ideas on this? I have a preference for the simplicity of the second one, but also am not so keen on the maintainer data being NULL. I'd also want to see the backfiller tested thoroughly, since that final drop would be inconvenient if it did the wrong thing.
Also, when I wrote #710 I wrote down these notes about migrations: https://www.notion.so/grahamc/How-to-make-a-database-change-123c16a7ea414b5eb45d27f8e11ca2d3
| <em>[% maintainer(m) %]</em>[% UNLESS loop.last %], [% END %] | ||
| [% END %] | ||
| [% END %] | ||
| </td> |
There was a problem hiding this comment.
Which page is this rendered on? I'm a bit unsure about how often we want to query and display this data, in particular I'm thinking tables which are very heavy with many rows of builds.
There was a problem hiding this comment.
Which page is this rendered on?
- Jobset evals (this are probably the biggest issue if
?full=1is set). - Search page
- Status page (running builds etc)
- "My builds" (if your email matches the one from your Hydra account you can already see maintained derivation-builds).
I'm a bit unsure about how often we want to query and display this data, in particular I'm thinking tables which are very heavy with many rows of builds.
While the biggest problem will probably be the jobset-eval page, this is the one where I really want to have this feature. I suggest to test this with live-data and try to optimize the query (and caching) if necessary.
|
@grahamc Thanks a lot for the detailed feedback!
Agreed. That's why I brought this up :)
I agree. Rather than having to keep the upgrade-path code in
This is actually the part I'm mostly anxious about. As soon as there's a working draft for |
|
Btw I'll try to implement the migration during the next weeks (as soon as I have time to) as we seem to agree to go for the second option. As soon as this is done, I'll try to do some live-testing to make sure that this doesn't explode with real-world data. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rss-feed-for-newly-failing-builds-from-hydra/8514/7 |
|
Any updates on this? It would be nice to have and closes NixOS/nixpkgs#101741 |
|
Just left a comment in the issue you've linked: I don't think that it should be blocked by this. When it's ready we can still adapt the script. Regarding the updates: it's still missing a sane migration path and I'm sorry, but I didn't have the time yet to work on that. Hopefully, I'll be able to finish that in December :) |
Ma27
force-pushed
the
jobset-search-maintainer
branch
from
34a4848
to
5b14e27
Compare
|
Just pushed a small batch of changes related to the DB migration. Currently, the following things are tbd to consider this ready: |
|
@grahamc would you mind taking a look at the migration so we can make a plan on how to test it? When I have more time & motivation for this again, I'd take care of your review comments from July :) |
|
@edolstra would you mind leaving a review here? :) |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
cc @NixOS/hydra-triage any chance to get a review here? :) |
|
@grahamc did you have a chance to take a look? :) |
In order to search for maintainers (emails or github handles), both values need to be stored. Also, it's probably not desirable to do this in a string-separated list which is fairly hard to query. Instead, those values will be stored in a separate table named `Maintainer` which is associated to the builds-table using a many-to-many[1] relationship. Please note that I didn't implement a migration for older builds yet, we probably want to implement an approach similar to the `hydra-backfill-ids`-implementation[2] here. Another benefit of this is that if a maintainer with a given email changes their GitHub-handle, Hydra will recognize this and display the new handle. [1] https://metacpan.org/pod/DBIx::Class::Relationship [2] f692601
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/calling-all-hydra-users/11851/7 |
|
@grahamc may I ask if you'd have some time for a review / test of this? I guess I may have some more timeslots to in the near future to e.g. take care of the next rebase and respond to comments / change requests :) |
Conflicts:
flake.lock
flake.nix
src/lib/Hydra/View/NixExprs.pm
src/root/jobset-eval.tt
src/script/hydra-eval-jobset
t/maintainers-upgrade-test.nix
Decided against a rebase here and t resolve everything in one go.
…tine Also explain which relation is resolved here. IMHO this DBIx DSL isn't the nicest one for this job.
|
This could use tests for at least the channel output and Email notifications btw. But the current testcase should cover most of the stuff that's relevant already :) |
Conflicts:
t/api-test.pl
This isn't needed since latest Hydra is quite recent and works fine with latest nix.
Conflicts:
flake.lock
src/lib/Hydra/Controller/Build.pm
src/lib/Hydra/Plugin/EmailNotification.pm
src/lib/Hydra/Schema/Result/Builds.pm
src/lib/Hydra/Schema/Result/Users.pm
src/sql/update-dbix.pl
src/sql/upgrade-75.sql
|
Since I was rebasing other branches anyways, I decided to just update this one as well :) cc @grahamc @cole-h if you'd like to review this and need some assistance, let me know! Note: even if we're finding the time to get this ready, I'd suggest to not use it for this ZHF period since it requires some database migration tasks that I'd rather not do at the time of the year, where the hydra.nixos.org has its most users :) |
|
@grahamc should we try to get this into a mergable state at some point now? %) |
Conflicts:
flake.lock
flake.nix
src/lib/Hydra/Controller/JobsetEval.pm
src/lib/Hydra/Controller/Project.pm
src/lib/Hydra/Schema/Result/Builds.pm
src/sql/upgrade-79.sql
Conflicts:
src/hydra-eval-jobs/hydra-eval-jobs.cc
src/sql/upgrade-82.sql
|
@grahamc, so I just did the following things (as we discussed.... quite a while ago):
IIRC the next step is to test this somewhere against hno, correct? In that case I'd probably need a test-instance since I'm pretty sure that none of my servers has sufficient disk-space (though I don't know the exact numbers, but I'd expect the db to be >100G at least, right)? |
Summary
Continuation of my efforts from #685.
During the release-managers' talk from last NixCon it has been suggested to add a search-feature to list all failing packages a user maintains. Theoretically this is already possible by introspecting the job-list in the dashboard of a Hydra-user, but not everyone owns a google-account (which is usually needed for the upstream-Hydra), also not everyone uses their google-email for NixOS stuff (I don't, for instance).
To summarize, the following things changed:
Instead of saving all maintainers' emails as a comma-separated string in the builds table (and therefore creating a lot of duplications), all maintainers are stored in a separate table and are associated to a build using a many-to-many relationship in DBIx. Whenever maintainers are needed in a controller-action, those can be fetched using an additional JOIN. For the REST-API, the maintainers are fetched by default when requesting details of a build.
This has the additional benefit that if the GitHub-handle of a user changes, Hydra updates the existing maintainer entry.
The filter-form in the jobset-eval view now contains an additional select-field to choose whether to filter by name or by maintainer (for backwards-compatibility, this is optional and by default it's filtered by job-name).
For a detailed overview, I suggest to review the commits including their messages.
Screenshots
The jobset-eval overview has been slightly changed to display and filter maintainers:
The detail-view of a build now displays the github-handles by default:
If a maintainer doesn't have a github-handle, the email is shown as fallback:
Pending work
maintainersfield from all entries in thebuilds-table which is bad if you need to search older jobsets or want to receive notifications if a job is restarted. I had the following ideas (though at least @grahamc might have more experience here):jobset_id-fixesmaintainers-column and merge the data from this column with the data from themaintainer-tablelib.maintainers.ma27). In that case it's even possible for Hydra to keep track of changes to the GitHub handle and email-address of a maintainer.cc @edolstra @grahamc