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
Replace jobset_input_alts with a JSONB field #375
Conversation
Why do we need multiple values at all anymore? |
@shlevy: In this particular case it's for setting properties on Git inputs to handle pull requests. And apart from that, we already have multiple values, for example for branches/deepClone, ids, job(set) matching and so on. So do you have a better solution to address this (except for EAV of course)? |
We now longer support SQLite (see 78974ab) and we already use a temporary PostgreSQL database during shell setup. So let's remove all conditionals about SQLite and all dependencies. However, we now have the problem that DBIx classes are now generated all-lowercase when running a DBIx update. The reason for this is that if you don't quote table names in SQL, the identifier is folded to lower-case and to upper-case according to the SQL standard[1]. SQLite on the other hand only matches case- insensitively but on a schema dump the tables are with cases. Let's illustrate this by example, on PostgreSQL we have this: hydra=# CREATE TABLE TestFooBar (foo int, bar int[]); CREATE TABLE hydra=# \dt test* List of relations Schema | Name | Type | Owner --------+------------+-------+-------- public | testfoobar | table | aszlig (1 row) And on SQLite we have the following: sqlite> CREATE TABLE TestFooBar (foo int, bar int[]); sqlite> .schema CREATE TABLE TestFooBar (foo int, bar int[]); While it's perfectly fine to reference tables using lower-case identifiers, DBIx's make_schema_at() will pick up the tables from the schema. So in the end, we need to fix all of our identifiers to get upgrades working properly again. Note that there still is a dependency on DBDSQLite in release.nix, because it is needed for the Perl bindings of Nix. [1] https://www.postgresql.org/docs/current/static/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS Signed-off-by: aszlig <aszlig@redmoonstudios.org>
We want to map underscore-separated tables to CamelCase so that we still have the same class names as we had so far. Unfortunately we can't use the default moniker, because it tries to make pluralized names singular. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
With the new naming scheme for database identifiers, we're now going to have build_steps instead of buildSteps, so we need to adjust the relation name mapping accordingly. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
We have rules to convert everything from the a_b variants to Perl class names, but because "SchemaVersion" will become "schemaversion", we'd get a class name called "schemaversion" instead of "SchemaVersion". This is in preparation of the conversion of all database identifiers. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Having CamelCase names for tables and columns doesn't really work out well, because we'd need to not only quote those names during creation but also all queries would need to use double quotes around these identifiers. Now every database identifier is consistent and lower-case only, which should make it easier to use within raw statements (and inside the SQL shell) and also less confusing in terms of schema definition versus raw statements or other code references. Of course, this isn't a very trivial change, so I wrote a small script which makes it easy to rebase this very commit should there be changes to master in the meantime. The Nix expression generating the script can be found here (yes, it's gory and a variant of "write-once-never-touch-again"): https://gist.github.com/aszlig/59ea813c28731d339b9764c46f363200 Note that I've left "userName" as-is (just making it lower-case in the schema) because there were a lot of references in the code that overlap with the column name. Another table I didn't touch except from making it lower-case (which it is anyway on PostgreSQL) is "schemaversion", because that table is needed for upgrading existing instances. Moreover, the reason why there now is no relation from jobset_inputs to jobsets (and back) anymore in the DBIx schema is that because it is now created using PostgreSQL instead of SQLite, that foreign key however was defined for SQLite only: > #ifdef SQLITE > , > foreign key (project, name, nixExprInput) > references JobsetInputs(project, jobset, > name) > #endif I've ran tests.install, tests.api and the tests in build.x86_64-linux and they all have passed. In addition to running those tests, I've dumped my running Hydra instance with database schema version 45 and tested the change. The upgrade worked successfully and the web interface is still working after a few tests. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Originally from 203a9b9. We want to track whether upgrades are consistent by using this as the base SQL file and iterating through all of the update SQL files, comparing the result against src/sql/hydra.sql. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Whenever we make a new upgrade-N.sql file, we need to make sure that we also add it to hydra.sql. That's exactly the part where human error can happen, so in order to mitigate this we now have a checker that can be called by: make -C src/sql check-db The update-dbix target has a dependency on check-db, so whenever we update the DBIx schema, we first check whether the singleton schema in hydra.sql is consistent with base.sql plus every update. We should now be able to find and fix already existing inconsistencies. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
The check constraint on jobsets (jobsets_check) has been explicitly named with underscores in upgrade-41.sql while the name within hydra.sql hasn't been set, so PostgreSQL will automatically create a name for the constraint, which luckily overlaps with our new naming scheme. Unfortunately however if a new Hydra instance has been set up between upgrade 41 and 50, the constraint would be implicitly created with the name jobsets_schedulingshares_check. In order to deal with this, I've added two statements in upgrade 50 that explicitly remove both variations of the constraint and recreate it with the name now defined in hydra.sql. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
These indices were added between introducing the schema upgrade system and were missed adding to upgrade-2.sql back then. The relevant commit is 8d65ab6. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
This once was added in upgrade-12.sql as IndexBuildOutputsOnPath, but never made it into hydra.sql, so let's make sure that upgrade-50.sql fixes this for existing systems that may or may not lack this index. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Both of these weren't properly picked up by apgdiff, so let's rename them as well. I think especially the rule isn't supported by apgdiff. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
PostgreSQL version 9.2 doesn't support renaming rules, so let's just drop rule idempotentinsert rule and recreate it with the new name idempotent_insert instead. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
I've missed this in 28d10b4 mainly because it didn't show up/failed in the tests. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
I'd recommend 9.5 though, but this is the minimum required version to support JSONB columns which we're going to use soon to replace jobset_input_alts. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
We could have used the classic entity-attribute-value (EAV) model instead but that would involve lots of self-joins and we'd also need to put every single property value into a series of text fields. If we want to have nesting, like we have for the "attrs" value for the "build" and "sysbuild" types we'd have to introduce even more madness by even creating yet another EAV table. Of course, we could avoid the nesting by serializing the stuff into the text fields, but serialization would be something like JSON anyway and we have a field type for that since PostgreSQL 9.3 and even a JSONB type in version 9.4 which is even indexable. The really nasty part here was getting every jobset input type into JSON here, because during a schema upgrade we can't rely on PL/Perl being available and everything outside seems to be hackish by modifying the schema updater code to run a Perl function just for upgrade 51 (which also needs to be retained). Right now, this only updates the database and the DBIx schema (also with deflation/inflation of properties from/to JSON) but not the code, which is still using jobset_input_alts and will obviously fail now. Signed-off-by: aszlig <aszlig@redmoonstudios.org> Issue: NixOS#279
The radiobuttons flag is no longer referenced anywhere in the code and the last bits were removed in e8cbcb5. So it's basically dead code, so let's remove it. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Switches the supportedInputTypes sub to use the new property system. The documentation in Hydra/Plugin.pm is not yet finished because the structure of the properties might change while I'm working on this. But in the end we now can specify input properties in a declarative way rather than parsing a single string field. Editing and displaying these properties currently still throws an error. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
We now have a hashmap called inputTypeNames we give to the template, so that we can simply pass this to renderSelection for the root type. For the properties themselves, we can currently render singleton types and multi-value types, including attrsets. However, we still don't handle optional fields yet. The UI design of these properties still needs to be revamped as well. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
When usisng OPTIONS on a jobset, we can get the declarative specification of the properties. This is not only useful for our internal UI, but is also useful for external applications, because they can validate these properties before sending them to our backend. Ideally, we would not only expose OPTIONS for just the properties but also for all the other fields of a jobset as well. But for now, let's focus on the properties only and expand on this later. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
9fd4a39
to
1284548
Compare
Rebased against current version of #355. |
Oh, I thought you were just reimplementing nput alts. OK, that's fine. |
eba96ff
to
ce6f14a
Compare
This is mainly a lot of JavaScript, because we don't know beforehand how many properties there are and also we have a property type for "attrset", which consists of arbitrary fields. Getting back the values of all those form fields, which could be arbitrary and nested is one the trickiest part here. The way this works is that the specification of these properties is retrieved into a static variable using an OPTIONS request on the jobset URL. Afterwards the specification is used for generating all the DOM nodes and the discharger functions. When the final form is submitted, the discharger functions are called for each property and they return the current values of the form field, which in turn gets serialized into JSON. However, the implementation is very basic and currently doesn't account for recursive attrset properties nor does it handle optional fields or help texts. Attrset properties currently are lacking an "add attribute" button to create a new key + value, so it's pretty useless right now unless you edit an existing attrset. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Now we have a button that can be used to add additional attributes to property fields of type "attrset". I've also set the class of the definition lists to "property", so it won't cripple the design too much. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Especially when property specifications get quite large, it's much more verbose to mark everything as optional. So we only need to specify mandatory properties via required => 1. The required attribute is implied for singleton properties. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
If the property has a "help", attribute the input element will get a corresponding title. Right now, we only have this for the deepClone property of the "git" input type. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
We now have a "validate" key for the property specification, which allows custom functions to check whether the input is valid or not. Right now, this isn't checked on the PUT method of jobsets yet. As the validation function can't be serialized, we also need to make sure it's stripped off during an OPTIONS request on jobsets. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
This replaces the previous checkInputValue sub with a new validateProperty sub that checks every property type for its validity and also calls the "validate" function if it exists. We now get a proper error message for both the API users and the web UI if the submitted input is wrong. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
This is on the side of the evaluator, where we didn't handle the new properties so far. The code removed is were the ugliest parts of the previous implementation, because they were trying to parse all those attributes using string matching. While working on this change, what particularly struck me was this: if ($exprType eq "guile") { if ($value eq "true") { $result = "#t"; } else { $result = "#f"; } $result = $value; } All of this is more or less a no-op, because $result gets overwriten by the original $value. So I've changed it to return "#t"/"#f" for Guile now. Signed-off-by: aszlig <aszlig@redmoonstudios.org> Cc: @civodul
I've ran all the tests currently exposed by release.nix and of course also the tests that are run in build.$system. All of the tests now succeed and a "git grep -i alts" only gives back src/lib/Hydra/Controller/API.pm as the last step that's left to convert to properties. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
I've removed this in e0a2656, thinking it wasn't needed because it was only referring to jobset_input_alts. Unfortunately, the json_hint is for deserializing the values of the tables to JSON. Now since we're using an inflated column, we can't simply use get_column() in TO_JSON anymore, because it will return the deflated (raw) version of the database value. So I've added another key for json_hint to designate inflated columns, which then get properly serialized back into JSON. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Before we have refactored everything into the new property system, the build value could have been an integer designating the build ID. The feature to designate a build ID has been broken during the last few commits and now gets reintroduced by splitting the "build" type into another type called "buildnr". The latter is just for the build ID, while the former accepts job, jobset, project and attributes. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
The documentation so far was quite wrong, because it was describing one of my experiments during the design of the property system. Now the documentation should be up to date and will hopefully be helpful enough to understand the system. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
It's already done in the backend (validateProperty) and we get a sane error message from the JSON result already so we don't need to do this check twice in the backend and the JavaScript UI. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
In particular, this allows specifying something like this: $inputTypes->{'git'} = { name => 'Git checkout', properties => { ... handlePR => { label => "Handle pull requests", type => "bool", properties => { token => { label => "Secret token", required => 1 } } }, ... } } This in turn will then instead of generating a single value for handlePR create a JSON object that contains something like this: { value: true, children: {token: 'sometoken'} } With that we can now nest properties as much as we like. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
This was the only thing left which was still using jobset_input_alts. We now use the ->> operator to do a lookup on a specific property. I've left out the match for jobset_inputs.type = 'git' in the non-GitHub version, because it was matching "value" from jobset_input_alts before and thus it stays generic enough to trigger jobset for non-Git SCMs. I've also added a status_ok() in the GitHub version, because otherwise we get an error page as the reply, even though the jobsets were triggered. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
34578d1
to
492399f
Compare
This property type is for values that only get exposed to the project owner and are not shown via the "Configuration" tab of the jobset or the API. An example for this would be a token for private Git repositories. The implementation could be more DRY in that we have a mapProperties function that could be used for validateProperties as well, but we'd have to restructure the whole validation functions again. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
While in general this wouldn't be a problem, because the values would be sent out as-is using the API it will become a problem as soon as type "secret" is used in a property and that property is no longer existing in the spec. It would then spit out all the secrets where there is no specification for the type, which is definitely not what we want. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
944e714
to
18a150f
Compare
Declarative jobsets require to enter a jobset input while creating a project, so we need to render the selection based on the new property system. This makes inputTypeNames in that we now handle this in renderSelection directly, because the latter is _only_ used for selecting jobset (and now project) inputs. Fixes rendering the jobset input selection while creating projects, although it doesn't use the property system for the values yet. Signed-off-by: aszlig <aszlig@redmoonstudios.org> Cc: @shlevy
In upgrade-51.sql, we have defined a few functions that help in converting simple values. Because this branch is not yet merged into master we can safely assume that nobody has deployed this in production yet. So we move dropping of these helper functions to upgrade-53.sql instead so that we can re-use these functions to migrate the "declvalue" in the projects table as well. We also apply the build->buildnr fix from upgrade-52.sql as well, although I find it unlikely that anyone out there using declarative jobsets is using that combination. Signed-off-by: aszlig <aszlig@redmoonstudios.org> Cc: @shlevy
All of the stuff handling properties is useful for editing/creating projects as well, because we do have one input field for declarative jobsets. So moving that stuff into a separate file makes sense and also makes edit-jobset.tt less crufty. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
As properties are not only going to be used for jobset inputs it really makes sense to move them to an URL which is always accessible. And because it's supposed to be not visited directly by a browser IMHO the best place for it is in /api. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
This is just to make it easier to work with the column the same way as it is done in Schema/JobsetInputs. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
It's not complete, because it currently doesn't check the validity of the properties. But now we can handle the declprops field the same way as any other jobset input field and thus have more consistency. I'm using a dummy input field "input-decl-name" here, because we don't need to rewrite the JavaScript that handles the properties to make room for properties with a fixed name. The reason this is there is that if you edit a jobset input you can also change the name of the input, which doesn't apply for creating/editing projects. Another thing that's changed by this is that we now submit the form values as JSON instead of URL-encoded, which not only makes it easier to encode but also easier to inspect via the browser's script debugger. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
We're specifically going to re-use the code for the Project controller but having all of the property handling in one place also provides a great benefit. No changes in functionality, just moving stuff around. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
If a boolean property is set to false it will default to null, which removes the field entirely. Unfortunately if the field is required it's dropped as well, which in turn leads to a validation error because the property is not defined but required. This should fix boolean singleton properties in that they now can be set to false. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
This now validates the properties when the project is edited or a new one is created, the same way as it is validated for jobset inputs. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
If a sub-property isn't enabled by its parent value (for example if the parent's property is optional), the check for discharged.value works. But if the value is a boolean, we get a JS error that we can't access a boolean value like an object. This is of course true, because we need to check the discharged value directly (it's not an object but the actual value). Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Using {} was a placeholder back then when first writing the implementation. So far I've solely used sub-properties for secrets so this didn't get to my attention. So if values are already present for a given sub-property the values are used when editing a jobset input or a project, otherwise it will fall back to {} again. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
This is a follow-up to bfdde81. Now that we no longer pass inputTypeNames as "options", we need to reference the .name attribute of that hashref in order to get the right description for the input. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
@shlevy: Missed that project edit/create has an input as well, so I've fixed it and generalized the code used for jobset inputs to be re-used for declarative jobsets. |
This builds upon #355 (thus the commits within this PR will be less cluttered after #355 is merged, otherwise please start at d00399badac55a9a5edeac6ae21cfedfbb54c325 with reviewing) and uses a single
JSONB
field to allow for multiple values for a specific jobset input.So far we already had multiple values, but those were stringly typed by either splitting them of or using regular expressions like this to tear apart the string into multiple values.
The result is something like this in the edit view of the jobset:
And this is how it looks like in the
Configuration
tab of the jobset:Fixes: #279
Cc: @edolstra