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

feat: enable Taps and Targets to generate their own Meltano yaml files #1094

Conversation

aaronsteers
Copy link
Contributor

@aaronsteers aaronsteers commented Oct 20, 2022

Update:

I've restructured this to build on #1096. That one would therefor need to merge before this one.

Timing:

  • I'd like to get this delivered before Tap-Toberfest if possible.

Todo (this PR):

Todo (future iterations):

  • Also detect and print setting nodes for sub-properties. (Currently, only top-level settings are documented.)
  • (Maybe) Allow developers to give hints to maintenance/maintainer-related metadata like 'repo', 'variant' and 'maintenance_status'.
    • As of now, I think it's fair to say these are too self-referential. Many are just generally problematic to add to the repo directly in code because all forks would inherit the same values by default. And a status like 'active' would also just not make sense as a permanent artifact of the repo.

@aaronsteers
Copy link
Contributor Author

@edgarrmondragon and @pnadolny13 - This is an early draft but feel free to add comments/suggestions. Thanks!

@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Merging #1094 (78d8aee) into main (253851e) will decrease coverage by 0.40%.
The diff coverage is 34.72%.

@@            Coverage Diff             @@
##             main    #1094      +/-   ##
==========================================
- Coverage   83.41%   83.00%   -0.41%     
==========================================
  Files          40       42       +2     
  Lines        3840     3889      +49     
  Branches      655      666      +11     
==========================================
+ Hits         3203     3228      +25     
- Misses        472      495      +23     
- Partials      165      166       +1     
Impacted Files Coverage Δ
singer_sdk/dev/cli.py 0.00% <0.00%> (ø)
singer_sdk/plugin_base.py 74.71% <17.85%> (-0.43%) ⬇️
singer_sdk/helpers/_meltano.py 83.33% <83.33%> (ø)
singer_sdk/helpers/_typing.py 51.55% <0.00%> (+1.24%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@aaronsteers aaronsteers changed the title chore: Enable Taps and Targets to generate their own Meltano yaml files feat: enable Taps and Targets to generate their own Meltano yaml files, add secret support in plugin config Oct 20, 2022
@aaronsteers aaronsteers changed the title feat: enable Taps and Targets to generate their own Meltano yaml files, add secret support in plugin config feat: enable Taps and Targets to generate their own Meltano yaml files; feat: add secret support in plugin config Oct 20, 2022
return None


def meltano_yaml_str(
Copy link
Collaborator

@edgarrmondragon edgarrmondragon Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronsteers So, I guess this is done as pure string manipulation so comments can be added.

Wouldn't it make more sense for Meltano to ingest the output of --about --format=json to generate the config it expects? I say it because the SDK shouldn't need to know about what fields Meltano expects, but Meltano could have a utility CLI (bundled in the same Python meltano package perhaps) to parse the SDK plugin metadata:

tap-github --about --format=json | meltano-parse-sdk-info

Another benefit of that approach is that developers (or us) don't need to upgrade their packages to be able to output the right Meltano config, rather users of Meltano bump to the latest meltano package to get this utility.

Developers could still bump the SDK version in their packages if we update the metadata exposed through the --about --format=json output.

Wydt?

Copy link
Contributor Author

@aaronsteers aaronsteers Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronsteers So, I guess this is done as pure string manipulation so comments can be added.

Wouldn't it make more sense for Meltano to ingest the output of --about --format=json to generate the config it expects? I say it because the SDK shouldn't need to know about what fields Meltano expects, but Meltano could have a utility CLI (bundled in the same Python meltano package perhaps) to parse the SDK plugin metadata:

tap-github --about --format=json | meltano-parse-sdk-info

Another benefit of that approach is that developers (or us) don't need to upgrade their packages to be able to output the right Meltano config, rather users of Meltano bump to the latest meltano package to get this utility.

Developers could still bump the SDK version in their packages if we update the metadata exposed through the --about --format=json output.

Wydt?

My intuition was similar at first, and certainly these are good points. Unfortunately, there just isn't a natural place for this to live in Meltano as of yet, and I think it will harder if we try to iterate both tools in lock step. I think we have an opportunity in the SDK to introduce a first round of interop, followed by quick patches if/when we find any issues. When stable and proven, we can migrate this functionality into Meltano - but I don't know that we yet have a good enough feature to make it worth adding to Meltano and then phasing out when we inevitably have a more robust paradigm. There just isn't anywhere I can imagine us putting this into Meltano as of today that wouldn't need to be removed/replaced in a future Meltano revision. But meanwhile, if we iterate in the SDK, we should get to a place where the issues are ironed out, and then we'll be closer to something like a meltano publish extractor or meltano create extractor type of a workflow.

All that said, I really don't think there is any issue with baking MeltanoHub interop directly into the SDK, since it directly helps developers promote their taps, and likewise streamlines adoption/onboarding for users even if the tap is not on the Hub. It's not intrusive and doesn't overly promote or favor Meltano over other orchestrators.

It's possible we'll phase out --about --format=meltano before we get to 1.0, but I think it's a valuable and (relatively) stable increment from where we are today.

Copy link
Contributor Author

@aaronsteers aaronsteers Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edgarrmondragon - Since our conversation, I've refactored this to an internal/dev-side CLI.

% sample-tap-countries --about --format=json > tap-countries.about.json
% singer-tools analyze --from-file=tap-countries.about.json --out-dir=.output
Meltano plugin definition: .output/sample-tap-countries.meltano.yml

This doesn't change the experience at all for tap users, but it makes a new command available to developers and power users who want to leverage it. As noted in poetry.toml, this can be executed from with the SDK repo during development, or by installing singer-sdk as a standalone program with pipx.

I don't think we should necessarily promote at this time, but developers can also execute this with python -m singer_sdk.dev.cli --help or python -m singer_sdk.dev.cli analyze ... anywhere that the singer_sdk library is installed.

@WillDaSilva - In the long run, I wonder if we can+should put the hub analyze work here in the SDK singer-tools dev CLI. One of the reasons the analyze command stores its output to a directory of files is that I am imagining we could move that complex /plugin-test logic here, and then let the Hub call this command - analyzing the tap or target and storing a bunch of file outputs based on the discovery and analysis.

Copy link
Contributor

@pnadolny13 pnadolny13 Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronsteers my initial reactions were the same as @edgarrmondragon's in that we're tightly coupling the hub and the SDK/Meltano CLI by baking it in. Not that we're planning to change the hub definition structure much but if we ever did make a change then it means we'll most likely get user contributing invalid definitions because they're a version out of date.

What if we use the EDK to make a utility that converts the --format=json output to a hub yaml file. It's not necessarily where I was going with it originally but I created a utility that helps create taps/targets hub definitions and puts them in the right location in the hub repo. If we had a meltano extension that contained this logic then we're not tied to the SDK version or meltano version either, the user could run meltano add utility hub-utils then meltano invoke hub-utils add and it kicks off prompts to get the --about scrapped, prompt for any other info like a logo png, and write the yaml file to the appropriate directory in the hub repo. If we make any changes to the format or whatever, the user just needs to re-install the updated utility to get support for those changes.

@aaronsteers aaronsteers changed the base branch from main to 77-feat-secrets-support-in-config-and-streams October 20, 2022 23:31
@aaronsteers aaronsteers changed the title feat: enable Taps and Targets to generate their own Meltano yaml files; feat: add secret support in plugin config feat: enable Taps and Targets to generate their own Meltano yaml files Oct 21, 2022
Base automatically changed from 77-feat-secrets-support-in-config-and-streams to main October 21, 2022 23:14
aaronsteers added a commit that referenced this pull request Oct 24, 2022
…) and `examples` (#1098)

* chore: initial refactor for readability

* feat: add Meltano rendering logic in private helper module

* feat: add `secret=True` support in JSON Schema type helpers

* change: update examples to use 'secret=True' for protected settings

* chore: flake8 fix

* add unit tests for type helpers

* fix missing secret flag on unit test

* chore: get tests passing

* chore: add test for description

* chore: remove commented code

* chore: remove files related to #1094

* chore: revert --about updates

* use constants for annotation keys

* chore: bump validator to Draft7

* chore: add testing for is_secret_type

* chore: add tests

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* chore: more tests

* docs: add info to FAQ

* chore: add test for integer type

* feat: add `allowed_values` and `examples` to Property class

* chore: add tests and samples

* chore: fix missing typing import

* docs: updated usage examples in typing module ref

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@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.

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