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

Replace jobset_input_alts with a JSONB field #375

Closed
wants to merge 51 commits into from

Conversation

aszlig
Copy link
Member

@aszlig aszlig commented Aug 4, 2016

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:

image

And this is how it looks like in the Configuration tab of the jobset:

image

Fixes: #279
Cc: @edolstra

@domenkozar domenkozar mentioned this pull request Aug 4, 2016
@shlevy
Copy link
Member

shlevy commented Aug 4, 2016

Why do we need multiple values at all anymore?

@aszlig
Copy link
Member Author

aszlig commented Aug 4, 2016

@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>
@aszlig
Copy link
Member Author

aszlig commented Aug 4, 2016

Rebased against current version of #355.

@shlevy
Copy link
Member

shlevy commented Aug 5, 2016

Oh, I thought you were just reimplementing nput alts. OK, that's fine.

@aszlig aszlig force-pushed the remove-input-alts branch 2 times, most recently from eba96ff to ce6f14a Compare August 5, 2016 09:26
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>
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>
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>
@aszlig
Copy link
Member Author

aszlig commented Aug 11, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of jobset alts
3 participants