-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
ServiceCatalog-AppRegistry Backend #8759
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
Conversation
bblommers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Peeanio! It looks like the tests in ServerMode are currently failing, see for example: https://github.com/getmoto/moto/actions/runs/14364128261/job/40273952245?pr=8759
See for the documentation on how to run the tests locally in ServerMode, hopefully that gives some insight into why it's not working:
https://docs.getmoto.org/en/latest/docs/contributing/development_tips/tests.html#servermode-tests
Let me know if you get stuck - happy to have a look as well!
| app = self._find_app_by_any_value(application) | ||
| if options is None: | ||
| options = [] | ||
| new_resource = AssociatedResource( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Backend also has an associate_resource-method which does the same thing, we would be the 'proper' way to do it - did you mean to use that instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - I think did this out of order and came back to this. Addressed in latest commits
| self.name = resource | ||
| cf_backend = cloudformation_backends[account_id][region_name] | ||
| try: | ||
| cf_backend.get_stack(self.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see an integration with other services immediately builtin 👍
|
Thanks for your comments. I took a look at the docs, but could not see how to address the 404's that appear in server mode. My intuition is that there's probably some mismatch in the backend index matching "servicecatalog-appregistry" vs "servicecatalogappregistry" not hitting the defined URLs, but I can't find it even with some changes. Any advice there would be appreciated. |
|
@Peeanio These are the changes necessary to make the build pass: Peeanio/moto@master...bblommers:servicecatalog-appregistry-fix-urls Explanation, in order:
|
ServiceCatalog AppRegistry: Fix URLs in ServerMode
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8759 +/- ##
========================================
Coverage 92.82% 92.82%
========================================
Files 1266 1271 +5
Lines 110646 110791 +145
========================================
+ Hits 102704 102843 +139
- Misses 7942 7948 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Changes looked good, thanks for the explanation. |
bblommers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thank you for adding this service to Moto @Peeanio!
Adding service catalog app registry backend. Supports creating/listing applications and associating resources. Associated resources can be tag value or Cloudformation stacks. AWS API raises "ResourceNotFoundException" when the CF stack is not present, but not sure how to search for the stack internally on the backend, or if it is required.
Happy to work through anything else missing.