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

Meltano lock fails on migrating to >2.0.0 when variant didnt previously exist #6360

Closed
pnadolny13 opened this issue Jul 5, 2022 · 7 comments · Fixed by #6534
Closed

Meltano lock fails on migrating to >2.0.0 when variant didnt previously exist #6360

pnadolny13 opened this issue Jul 5, 2022 · 7 comments · Fixed by #6534
Assignees
Labels
Documentation Improvements or additions to documentation kind/Bug Something isn't working valuestream/Meltano

Comments

@pnadolny13
Copy link
Contributor

We should update the log message and add a section to the migration docs to help users get variants added for plugins that didnt have a variant previously. For example Airflow locking fails because a variant isnt set for all installations prior to 2.0.0. The error message isnt totally clear in saying this and users will need to manually update their meltano.yml.

Originally discussed in #6359 (comment)

Additionally I'm getting Orchestrator 'airflow' variant 'original' is not known to Meltano. Variants: ['apache (default)'] from a default installation of Airflow. I know that its hard because when I originally installed it there wasnt a variant name for airflow but now there is. Is there a way for us to resolve that its the default variant? Maybe that wont work because the default could change, maybe using the discovery.yml when we think were migrating to >2.0.0?

Or we could log it better and say something like "I notice youre migrating and you dont have a variant, go to the Migration guide and learn how to define a variant" and we write up the steps for locking post migration and adding a variant name:

remove executable/namespace if defined
add variant using one available on MeltanoHub. We can list the mapping like airflow -> apache, dbt -> dbt-labs, etc. for plugins that just got variant names
run meltano lock --all

cc @edgarrmondragon

@DouweM
Copy link
Contributor

DouweM commented Jul 5, 2022

@edgarrmondragon @pnadolny13 Alternatively, could we make the meltano lock smarter when there is no explicit variant and there is only one defined on the Hub/discovery.yml? We could write variant: apache explicitly in meltano.yml and generate the lockfile as usual, instead of requiring the user to take the manual step of finding out that the only Airflow variant is apache.

That's essentially to what meltano invoke etc already do, which is why they still worked in this pre-2.0 Meltano project even with 2.0+, and I think we can make meltano lock "just work" as well.


Also:

remove executable/namespace if defined

I don't think we should recommend this: namespace is still supported and valuable, as it indicates custom plugins that shouldn't be looked up on the Hub (see also #3296 (comment) about why we can't just remove that property).

@pnadolny13
Copy link
Contributor Author

I don't think we should recommend this: namespace is still supported and valuable, as it indicates custom plugins that shouldn't be looked up on the Hub (see also #3296 (comment) about why we can't just remove that property).

@DouweM yeah thats true - I dont know all the details of executable/namespace but what I was calling out was that I had some plugins defined that were previously custom but are now available on the hub so when I run a "lock --all" they get skipped. I think because their namespace/executable are present and its currently defined as a custom plugin. We'd want to encourage users to migrate from custom to "hub managed" for all of those, as long as theyre not still custom (like from a private repo or not listed on the hub). Wouldnt removing namespace/executable be the way to do that?

I just want to make sure people are not accidentally making their installed plugins look like custom ones where they wouldnt get locked or benefit from upgrades on the hub. Maybe in this new migration guide section we can just make another note to reiterate that custom plugins dont get locked because they dont use the hub and that if you include keys like executable/namespace (are there others?) Meltano will treat it as a custom plugin and not lock it. Reiterating also that most plugins are available on the hub now so users should switch to those vs custom installations.

Does that make sense?

@tayloramurphy
Copy link
Collaborator

Alternatively, could we make the meltano lock smarter when there is no explicit variant and there is only one defined on the Hub/discovery.yml?

@DouweM I think for the short-term it makes sense to mainly update the docs as new users from 2.0 on will always have a variant for any new plugin. While I like the idea of making it smarter it'd be quicker to get an update to the docs listing the main plugins folks have to update.

@edgarrmondragon would you be willing to make the docs PR as well?

@tayloramurphy tayloramurphy added Documentation Improvements or additions to documentation valuestream/Meltano labels Jul 6, 2022
@DouweM
Copy link
Contributor

DouweM commented Jul 6, 2022

@pnadolny13 That makes sense, I wasn't thinking about the taps/targets that are now on the Hub but weren't previously. Your recommendation on what to document sounds good.

@tayloramurphy Good point on doing the quickest thing. If we add "set an explicit variant" to the migration guide, we may not need the better log message as people shouldn't see it anymore, but if someone misses that step I think it'd still be confusing that everything works fine, except for meltano lock, and the error message wouldn't really help them figure out what step they missed. So if we don't make it smarter, I think it's still worth making the log message more helpful instead of only updating the docs.

@tayloramurphy
Copy link
Collaborator

@DouweM makes sense to me. Log messages are kind of bucketed under documentation in my head a bit too.

Scope of the issue:

  • Update 2.0 migration guide with instructions on adding the variant
  • Update error message to make it clearer what's happening

@edgarrmondragon edgarrmondragon self-assigned this Jul 8, 2022
@aaronsteers
Copy link
Contributor

@edgarrmondragon - Do you have confidence in what would be needed on the docs side here, and then some helpful update to the error text that could point to the assist in the docs?

@edgarrmondragon
Copy link
Collaborator

edgarrmondragon commented Jul 29, 2022

@edgarrmondragon - Do you have confidence in what would be needed on the docs side here, and then some helpful update to the error text that could point to the assist in the docs?

@aaronsteers yes, I think we can either

  1. document that plugins should declare an explicit variant in meltano.yml or
  2. treat the original variant as we do the default, and just pull the default variant from Hub (I hadn't realized we can do this)

The latter would make this a bug fix rather than a mere documentation task. It should be fairly safe AFAICT, but we can still emit warning. Wdyt?

cc @tayloramurphy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Improvements or additions to documentation kind/Bug Something isn't working valuestream/Meltano
Projects
None yet
5 participants