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

Add settings 'write-back' capability (Singer SDK) #106

Open
MeltyBot opened this issue Apr 15, 2021 · 4 comments
Open

Add settings 'write-back' capability (Singer SDK) #106

MeltyBot opened this issue Apr 15, 2021 · 4 comments

Comments

@MeltyBot
Copy link
Contributor

MeltyBot commented Apr 15, 2021

Migrated from GitLab: https://gitlab.com/meltano/sdk/-/issues/106

Originally created by @aaronsteers on 2021-04-15 21:08:05


Related to similar issue for Meltano: https://gitlab.com/meltano/meltano/-/issues/2710

Some tap and target implementations use a writeback method (we think?) to store settings (such as auth tokens) back in the config.json file for future use.

There's currently no similar way for a tap developer using the SDK to write back settings for use in future invocations.

Wanted:

For this issue, we are currently gaging feedback and asking for developers to provide use cases if this is a blocker for them. If this affects you, please post to the comments.

Related

@MeltyBot
Copy link
Contributor Author

@stale
Copy link

stale bot commented Jul 18, 2023

This has been marked as stale because it is unassigned, and has not had recent activity. It will be closed after 21 days if no further activity occurs. If this should never go stale, please add the evergreen label, or request that it be added.

@stale stale bot added the stale label Jul 18, 2023
@stale stale bot removed the stale label Jul 18, 2023
@edgarrmondragon edgarrmondragon moved this to To Discuss in Office Hours Mar 19, 2024
@pnadolny13
Copy link
Contributor

@edgarrmondragon I implemented a version of this with the SDK in tap-gohighlevel.

  • I had to override the tap init to capture the config file path and save it to the tap instance. I also chose to raise an exception if the configs are passed in outside of a config file.
  • Then when my authenticator is being instantiated by pull the config path variable out and pass it in. The way I access that var is definitely not nice but it works.
  • I overrode update_access_token to catch the new refresh token
  • Then call a method _write_back_to_config that overwrites the file.

I honestly didnt spend a lot of time thinking through the cleanest way to pass this stuff around so let me know if theres a better approach.

@edgarrmondragon
Copy link
Collaborator

Thanks for sharing the implementation @pnadolny13!

  • I had to override the tap init to capture the config file path and save it to the tap instance. I also chose to raise an exception if the configs are passed in outside of a config file.

Yeah, I hadn't thought about it but more multiple --config values or --config=ENV don't really work with write-back, and write-back is a hard requirement for tap so it makes sense to straight-out fail.

That's a not a problem for Meltano because it always uses a (single) config file under the hood, and I'm willing to bet close to no one uses multiple --config flags.

That implementation does not seem to handle cases where --config=ENV is used to pass the refresh token, though.

  • Then when my authenticator is being instantiated by pull the config path variable out and pass it in. The way I access that var is definitely not nice but it works.

Cool! Though I don't particularly love create_for_stream cause in general it does not provide any benefits over a regular __init__ but it couples the stream and authenticator classes in a circular manner 😅.

  • I overrode update_access_token to catch the new refresh token
  • Then call a method _write_back_to_config that overwrites the file.

This combination makes me want to add a hook somewhere in the SDK that lets developers send a signal that the input config file should be updated.


I think the implementation makes sense, since it works well with Meltano's standard config location (as opposed to writing back to an arbitrary file) and it gives a reference implementation to adapt upstream here.

Unfortunately meltano/meltano#2660 is certainly harder to reason about and implement, but we can get something out for the SDK with this as a start.

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

No branches or pull requests

3 participants