-
Notifications
You must be signed in to change notification settings - Fork 26
feat: create, update, and delete integrations (stacked version) #440
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
feat: create, update, and delete integrations (stacked version) #440
Conversation
This reverts commit 1582d27.
We’re only going to be converting lists that we get from the server to integrations; we don’t need to use method dispatch here.
|
Looks like CI will not run until the parent PR is closed and this targets |
…ypick # Conflicts: # NAMESPACE # NEWS.md # R/integrations.R # man/as_integration.Rd # man/get_associations.Rd # man/get_integration.Rd # man/get_integrations.Rd # man/set_integrations.Rd # tests/testthat/test-integrations.R
| #' @param name A descriptive name to identify the integration. | ||
| #' @param description Optional, default `NULL.` A brief description of the integration. | ||
| #' @param template The template to use to configure this integration (e.g., | ||
| #' "custom", "github", "google", "connect"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to API docs for where you can get a complete list of supported values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, whatever is best. Just flagging that we should provide users with a way of knowing what the valid values here are.
| #' @param template The template to use to configure this integration (e.g., | ||
| #' "custom", "github", "google", "connect"). | ||
| #' @param config A list containing the configuration for the integration. The | ||
| #' required fields vary depending on the template selected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to API docs?
tests/testthat/2025.07.0/__api__/v1/oauth/integrations/60586f1c-b90ada-PATCH.R
Outdated
Show resolved
Hide resolved
Co-authored-by: Neal Richardson <[email protected]>
tests/testthat/test-integrations.R
Outdated
|
|
||
| with_mock_dir("2024.12.0", { | ||
| test_that("integration creates a single integration", { | ||
| test_that("get_integration() creates a single integration", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"creates"? or just "gets"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah lol you're exactly right, I was thinking in a different frame when I originally wrote it
| config = config | ||
| ) | ||
| ) | ||
| as_integration(result, client) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we're returning an actual object on a PATCH method? I wouldn't expect a function updating an integration to return the new integration object but maybe that's a reasonable side effect in the context of this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the reasons are two-fold. One is that it matches the way that if you're working with local R objects, the way you update an object is to call a function on it and assign that result to something -- like:
animals <- c("cat", "dog")
# update animals
animals <- c(animals, "bat")In the context of integrations and Connect, it matches the way the PATCH API works (it returns the updated object, and we're just passing it on.)
So if you imagine that in the context of a script, if I'm say, updating an integration, like giving a GitHub integration a new key, it makes sense for my local representation of that object to be updated to the new version.
client <- connect()
github_integration <- get_integrations(client) |>
purrr::keep(~ .x$template == "github")[[1]]
github_integration <- update_integration(github_integration, config = list(key = NEW_KEY))
# Now this object will reflect the updated info.
github_integration$config$keyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, makes perfect sense. thanks!!
33c7499 to
86617bb
Compare
6ca5987 to
a2c6741
Compare
|
I'm going to merge this despite CI failing, since it's broken for an unrelated reason. I'll fix CI in a separate PR and won't hold this up for it. |
Intent
Add functions to create, update, and delete integrations.
N.B. This supersedes #439, which was based on
main, and did not include changes from #437.Fixes #434
Approach
create_integration()takes aclient.update_integration()anddelete_integration()take aconnect_integrationobject as their first argument.connect_integrationclass now gets a client as an attribute when it is created.as_integration()no longer uses method dispatch.Checklist
NEWS.md(referencing the connected issue if necessary)?devtools::document()?