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: Feature flag to disable calls to Hub and discovery.yml outside of add and discover #7097

Merged
merged 13 commits into from
Apr 13, 2023

Conversation

edgarrmondragon
Copy link
Collaborator

@edgarrmondragon edgarrmondragon commented Dec 16, 2022

Closes #7095

  • Tests
  • Docs

@netlify
Copy link

netlify bot commented Dec 16, 2022

Deploy Preview for meltano canceled.

Name Link
🔨 Latest commit 47455c8
🔍 Latest deploy log https://app.netlify.com/sites/meltano/deploys/6437f77539ce520008de8602

@WillDaSilva WillDaSilva self-requested a review December 16, 2022 01:27
@aaronsteers
Copy link
Contributor

@edgarrmondragon - I think we have to hold this until 3.0, if it would break existing users (and there are a lot) who have not yet locked their plugin files. Can you confirm?

@edgarrmondragon
Copy link
Collaborator Author

@edgarrmondragon - I think we have to hold this until 3.0, if it would break existing users (and there are a lot) who have not yet locked their plugin files. Can you confirm?

@aaronsteers yes, invoking a plugin that isn't locked would result in a failure with this approach

@aaronsteers aaronsteers added this to the Meltano 3.0 milestone Feb 9, 2023
@edgarrmondragon
Copy link
Collaborator Author

edgarrmondragon commented Feb 9, 2023

@aaronsteers Perhaps this can be introduced earlier behind a feature flag?

@aaronsteers
Copy link
Contributor

@aaronsteers Perhaps this can be introduced earlier with a feature flag?

Great idea!

Maybe something like require_plugin_lock_files or plugin_locks_required?

I also considered a generic for:

As someone noted in Slack, a possible mechanism for this could be forcing hub_api_root and discovery_url both to be false.

That generic solve to restrict network connectivity would break new plugin additions though, so I ruled it out as a good solve here. The ideal state in 3.0, I think, is that you wouldn't be able to run plugins unless they had a local definition file. Making an opt-in flag now for this requirement is nice, because we can start recommending this flag as a best practice, and when we get close to 3.0 we can recommend it as a pre-upgrade step.

@WillDaSilva
Copy link
Member

Perhaps this can be introduced earlier behind a feature flag?

Sounds good to me.

because we can start recommending this flag as a best practice, and when we get close to 3.0 we can recommend it as a pre-upgrade step.

@edgarrmondragon @aaronsteers We could also make not having it enabled a lint issue:

@edgarrmondragon edgarrmondragon force-pushed the fix/disable-hub-no-add-lock branch 2 times, most recently from c15d599 to 45a5013 Compare February 10, 2023 21:12
@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #7097 (76b163f) into main (b2a13c3) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 76b163f differs from pull request most recent head 47455c8. Consider uploading reports for the commit 47455c8 to get more accurate results

@@            Coverage Diff             @@
##             main    #7097      +/-   ##
==========================================
+ Coverage   89.38%   89.40%   +0.02%     
==========================================
  Files         294      294              
  Lines       21658    21684      +26     
  Branches     2422     2426       +4     
==========================================
+ Hits        19358    19387      +29     
+ Misses       1939     1938       -1     
+ Partials      361      359       -2     
Impacted Files Coverage Δ
src/meltano/cli/lock.py 91.52% <100.00%> (+0.14%) ⬆️
src/meltano/core/project_add_service.py 95.12% <100.00%> (ø)
src/meltano/core/project_plugins_service.py 92.51% <100.00%> (+1.50%) ⬆️
src/meltano/core/settings_service.py 97.87% <100.00%> (+0.43%) ⬆️
src/meltano/core/utils/__init__.py 92.36% <100.00%> (-0.03%) ⬇️
tests/meltano/cli/test_add.py 95.76% <100.00%> (ø)
tests/meltano/core/test_project_add_service.py 95.94% <100.00%> (ø)
tests/meltano/core/test_project_plugins_service.py 100.00% <100.00%> (ø)

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

@edgarrmondragon edgarrmondragon force-pushed the fix/disable-hub-no-add-lock branch 2 times, most recently from cdb6e39 to ad430a8 Compare February 13, 2023 22:17
@aaronsteers aaronsteers changed the title fix: Disable Hub calls outside of add and discover feat: Feature flag to disable calls to Hub and discovery.yml outside of add and discover Feb 13, 2023
@aaronsteers
Copy link
Contributor

Not a high priority per se, but I've updated the PR description to reflect the feature flag-based scope discussed above, which enables this to ship before 3.0 without breaking existing users.

@edgarrmondragon edgarrmondragon force-pushed the fix/disable-hub-no-add-lock branch 2 times, most recently from 9d66585 to 8262163 Compare February 14, 2023 01:56
@edgarrmondragon edgarrmondragon force-pushed the fix/disable-hub-no-add-lock branch 3 times, most recently from 2c0c37b to 9afee75 Compare February 28, 2023 19:40
@edgarrmondragon edgarrmondragon marked this pull request as ready for review February 28, 2023 20:13
Copy link
Collaborator

@tayloramurphy tayloramurphy left a comment

Choose a reason for hiding this comment

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

Minor docs change, otherwise approving.

docs/src/_reference/settings.md Outdated Show resolved Hide resolved
Copy link
Member

@WillDaSilva WillDaSilva left a comment

Choose a reason for hiding this comment

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

This is great! I have one request, but aside from that this looks good to go

src/meltano/core/project_plugins_service.py Show resolved Hide resolved
src/meltano/core/bundle/settings.yml Show resolved Hide resolved
@WillDaSilva WillDaSilva merged commit bb58465 into main Apr 13, 2023
38 checks passed
@WillDaSilva WillDaSilva deleted the fix/disable-hub-no-add-lock branch April 13, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature flag to disallow Meltano calling Hub outside of add and discover
4 participants