Inconsistent stages API between GET and PATCH
Closed this issue · 11 comments
The Samson API differs in the data that is returned from a GET /stages/stage.json` and what you can supply in a POST/PATCH request. This inconsistency in the API makes it difficult to build an idempotent Ansible modules because I need to be able to compare what values the user has provided and the values that exist in the remote state. Because the GET request hides parameters in the stage it's impossible to compare the local and remote state.
E.g you can PATCH command_ids: [1,2,3], but when you GET a stage it does not contain the command_ids property. The same applies for slack webhooks and a few other properties.
Yes I can. Ansible uses a http client under the hood. However I wonder what the use case is exactly? The JSON endpoints don't affect normal users page loads (html), but is used by programs that access the Samson API. Do you guys use some sort of automated tooling at Zendesk that already uses these endpoints and needs to be quick?
So the issues I see with it are:
- Additional complexity
- More inconsistency in the API endpoints, e.g projects and commands wouldn't have this extra parameter but stages would.
Let me know what you think.
The basic issue is that there are lots of parameters that can be passed into the stage and returning all of them would mean rendering lots of the stages associations. Rendering association is normally done via includes, and we want index to be [show, show, show, show] which then would make index very big/slow :(
... so there needs to be an inconsistency somewhere, either it's index!=[show] or show gets an additional ?return_all_attributes=true parameter where we then go over all assignable associations and render them inline (and not how we normally do include)
If consistency would require a major overhaul of the API I think it's better to use your proposition in the short term and figure out a longer term plan if more users consider the API complex to use :)
include_all=true or include_level=N would work for more granular payloads
The HTTP client is thicker than I would like it to be because it needs to handle the inconsistencies, but I'll be reporting them here over time because I think there are a lot of small issues that can easily be fixed.
👍
@grosser I've outlined the API issues I've found so far when building Ansible modules for projects, stages and commands.
Ansible modules should ideally be deterministic and by that I mean no updates should be performed if the local state and remote state is different. Terraform works similarly.
Ansible modules usually have one of two states: absent or present. present indicate that a resource should be created or updated if it already exists. If the resource does not need updating the Ansible module should exit with changed=False.
absent indicates that the resource should be destroyed. If the resource is already absent in the remote state Ansible should exit with changed=False.
In order for Ansible to know if it should create or update a resource it needs a stable identifier to work with. For projects and stages I've used the permalink property, but commands are not permalinkable so I've had to hack around this. More on this in the below paragraph.
Environments and deploy groups don't have JSON endpoints
I would guess that most development teams that use Samson have a sufficiently complex infrastructure that has multiple deployment environments (prod, staging, QA). This indicates that stages will belong to one or more environments and deploy groups. Since I can't programmatically create environments and deploy groups using the JSON endpoints we need to use a hybrid approach of manual creation and automated setup.
Projects don't use the provided permalink on creation
Projects don't use the provided permalink on creation, even if one is provided. Instead they generate a random permalink formed by the repository path and a random suffix (e.g danihodovic/dotfiles-32313sad). This is unexpected behavior by the API and I work around this by performing a second update after the initial creation has been done.
Stages don't use the provided permalink on creation
Similar to the project issue above, but instead of using a randomly generated permalink it uses the stage name and replaces any whitespace and underscores by dashes. The workaround is the same - a second update after the initial creation.
Stages use a nested stage key to perform updates
-
POST /projects/<project-permalink>/stages.jsonspecify the stage properties at the top level. -
PATCH /projects/<project-permalink>/stages/<stage-permalink>.jsonuse a nested stages object within the payload.
This is inconsistent and unexpected behavior of the API.
Stages fail silently if the command ID is invalid
POST /projects/<project-permalink>/stages.json
...
{
"command_ids": [1, 2]
}
If one of the command IDs doesn't exist the API doesn't fail the request. Instead it creates a project local command with command.command being the ID provided, e.g 1.
Invalid permalinks fail silently
Providing an invalid permalink to Samson causes the API to replace the invalid parts with dashes. E.g providing whitespace makes Samson use the permalink, but use dashes in place of whitespace. I would expect the Samson API to reject such requests with a 400 error.
To work around this problem I perform client side validation on the permalink, because I need to use the permalink the user provides as an identifier.
Commands don't have an identifier on POST / PUT / PATCH
Commands are not permalinkable and instead use IDs as identifiers. The problem is that you can't create a command with a specific ID as it's randomly generated. This makes deterministic creation and updates impossible when using the ID as the identifier.
The creation of a command disallows POST operations on a specific path, e.g POST /commands/5.json would fail with a 404. Instead you need to use POST /commands.json and have a random ID returned. Since the user can't specify this beforehand I can't ensure updates are idempotent because running Ansible twice will create the same command twice instead of updating it in place.
I work around this solution by using a combination of command.command and command.project_id as the identifier. I list all commands that exist with the combination the user provided and create / delete based on that. There are no updates for commands.
For state=present I list all commands with the specific command.command and command.project_id combination. If such a command already exists I exit with changed=False. If it doesn't exit I create a new command.
For state=absent I list all commands and delete any commands that have the combination of command.command and command.project_id that the user provided. This ensures the module is deterministic, but unfortunately may delete any of the commands created outside of the Ansible module that match the provided parameters.
Conclusion
Not all of the above needs fixing and some of it is probably impossible to fix because it's backwards incompatible or break the UI flow. Client libraries can instead work around these issues even if it's not pretty.
The changes I'd like to see the most are:
-
- JSON endpoints for environments and deploy groups
-
- Stricter API behavior so that obviously invalid resources are rejected. Examples are invalid permalinks or missing command IDs for stages.
I'll report back once we roll out the Ansible module to more than one project if there are additional issues.
@grosser any idea how I can deterministically create / update Slack webhooks?
The following request will create two webhooks. I would prefer it to create it once and update it a second time.
$ stage='{"stage": {"slack_webhooks_attributes": [{"webhook_url": "http://foo.com", "on_deploy_success": true}]}}'
$ echo $stage | http PATCH http://localhost:3000/projects/myproject/stages/mystage.json 'Authorization: Bearer token'Need to send in the id too, not sure if that will fit your workflow.
... might need to add a option to render out slack_webhooks_attributes
I'd call this inlines to differentiate it from the includes which work by adding them alongside the stages.