-
Notifications
You must be signed in to change notification settings - Fork 149
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
Proposal: convenience mutations for the creation of advanced tasks #3400
Comments
I'd like to also see some improvements to the task arguments to cater for additional cases
Additionally to point 2, a type that could be
EDIT: I've raised #3477 |
Thanks for this, @shreddedbacon - I think that both of these suggestions can be factored into the current structure. Perhaps we spin this comment into another discussion/proposal. Should be easy to implement as well. |
If it can be done external to this proposal, I'll just create a feature request issue to get them added. |
Note: reference/POC implementation given in #3401
Motivation
The mutation for creating advanced tasks is presently quite difficult to understand.
Let’s take a look at the input type for the
AddAdvancedTaskDefinition
mutation:The following fields are fairly straightforward:
name - the Task’s name
description - a string describing what the task does
permission - the role required to run the task.
advancedTaskDefinitionArguments - the arguments that can be passed to / are required by the task
confirmationText - text that is optionally displayed in contexts like the UI before running the task
Complexity, primarily because of ambiguity, is introduced by two subsets of fields.
Firstly, there are the fields
The ambiguity in the API here is that if one chooses to create an IMAGE type, then the “image” field should be filled. Likewise, if one creates a COMMAND type, then the “command” field should be filled.
This is in no way obvious when looking at the mutation, which makes it possible to, say, specify an IMAGE and fill in the “command” field (or both the “command” and “image” fields)
Secondly, there are the fields
These three fields specify the target (or range of targets) of the task. Either a group (where all projects in the group will inherit the task), a project (where all environments in the project will have access to the task), or an environment directly.
The ambiguity here is that it seems as though one can set any combination of targets (or none) - whereas only one of the three should be set.
Both of these ambiguities are manifested in the fact that the mutation’s backend, rather than the type itself, is responsible for validating the incoming data.
The aim, then, is to disambiguate the type itself in such a way that the use of the mutation(s) are made obvious, thus also reducing the number of constraint checks in the backend as well.
Proposed solution
As a first step, leaning on the api structure and types would go a long way to contraining the users’ degrees of freedom, and thereby eliminate the ambiguity of the current method of defining custom tasks.
One way of achieving this would be to structure the mutation api via namespacing (see reference) and the creation of two new mutations specifically for the two types of advanced tasks.
The two new input types would be as follows
As can be seen above - these two remove all ambiguity about what is required to create an IMAGE or COMMAND type both by removing all extraneous fields for the type, but also by setting required fields as mandatory.
Finally, the ambiguity around specifying the target is fixed by introducing the following type
The user will create the new task with either project name (which is required, and thereby guarantees that there will be a target), or with project and environment name.
Note, further, that there is no
groupName
target, so this mutation will also remove the problems described in 1Example call
References
https://www.apollographql.com/docs/technotes/TN0012-namespacing-by-separation-of-concern/
https://khalilstemmler.com/blogs/graphql/nested-graphql-resolvers/
Footnotes
group level tasks should be deprecated immediately - there exists potential problems with namespace clashes. We should warn users not to use these and have “group level” functionality reintroduced with Organizations. ↩ ↩2
The text was updated successfully, but these errors were encountered: