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

CFn: allow for pro resource provider overrides #9333

Merged
merged 7 commits into from Nov 1, 2023

Conversation

simonrw
Copy link
Contributor

@simonrw simonrw commented Oct 11, 2023

Motivation

We disabled AWS::ECR::Repository in #9315 because we did not have an override system for resource providers in community vs pro. In particular, we had:

  • a community GenericBaseModel subclass
  • a (new) community ResourceProvider
  • a pro GenericBaseModel subclass

In this case, the community resource provider was being chosen, rather than the pro GenericBaseModel subclass.

Changes

Most of the changes to allow us to revert #9315 have been made in the pro repository. In addition, we are trying to handle the case where we have pro and community resource providers.

  • Prioritise pro resource providers if present
  • Update scaffolding to support scaffolding into the pro repo (partially, only the plugin for now)
  • Fix scaffolding issues with unused imports

TODO

  • allow for scaffolding into the pro repo

@simonrw simonrw added the semver: patch Non-breaking changes which can be included in patch releases label Oct 11, 2023
@simonrw simonrw self-assigned this Oct 11, 2023
@coveralls
Copy link

coveralls commented Oct 11, 2023

Coverage Status

coverage: 82.473% (-0.01%) from 82.484% when pulling 40bb7fe on cfn/resource-provider-override into 2991501 on master.

@github-actions
Copy link

github-actions bot commented Oct 11, 2023

LocalStack Community integration with Pro

       2 files         2 suites   1h 6m 56s ⏱️
2 311 tests 1 742 ✔️ 569 💤 0
2 312 runs  1 742 ✔️ 570 💤 0

Results for commit 926cee0.

♻️ This comment has been updated with latest results.

@simonrw simonrw marked this pull request as ready for review October 12, 2023 13:49
@simonrw simonrw marked this pull request as draft October 12, 2023 13:51
@simonrw simonrw marked this pull request as ready for review October 12, 2023 21:02
Copy link
Member

@pinzon pinzon left a comment

Choose a reason for hiding this comment

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

Awesome!!

@simonrw simonrw force-pushed the cfn/resource-provider-override branch 2 times, most recently from 0f14be8 to 980dd44 Compare October 23, 2023 15:16
Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

I'm not 100% sure I like the approach with two different plugin managers, since it feels like we shouldn't need it.

An alternative could be appending a certain suffix the the plugin name when coming from ext and then handling that logic in the community plugin manager directly.

For now I don't really want to block this too much, especially since we can re-evaluate this setup at a later stage without any user-facing changes. So feel free to merge as is!

Comment on lines 773 to 778
LOG.debug(
(
"Failed to load resource type %s from pro as a ResourceProvider. "
"Maybe this resource is implemented in community."
),
resource_type,
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this is a bit too noisy for community usage.

@simonrw simonrw force-pushed the cfn/resource-provider-override branch 2 times, most recently from b5f4c84 to 40bb7fe Compare October 29, 2023 21:00
@simonrw simonrw force-pushed the cfn/resource-provider-override branch from 40bb7fe to 926cee0 Compare October 30, 2023 16:18
@simonrw simonrw merged commit 64207b5 into master Nov 1, 2023
27 checks passed
@simonrw simonrw deleted the cfn/resource-provider-override branch November 1, 2023 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants