-
-
Notifications
You must be signed in to change notification settings - Fork 164
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
[BUG] The bundle command does not bundle referenced files #1323
Comments
Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request. |
I know this is not too helpful, but wanted to share that I am observing the same problem here as well, using |
@Souvikns @Shurtu-gal |
@peter-rr I've updated the related PR slightly regarding the way assertions are done, and also which referenced files etc are used for comparison. I spent some time trying to get to the root cause, but I am suspecting it's originating from https://github.com/asyncapi/bundler. This repo uses So this requires deeper investigation and also a plan for upgrade. |
Hey @francocm , thanks for the updates on the PR 🙌 I'm having a look at them! Yeah, the root cause must be definitely at the |
Cool @Shurtu-gal 👍 Thanks for the info! |
I have pulled from upstream into https://github.com/francocm/asyncapi-cli/tree/fix/1323/bundleNotBundlingExternalFiles and went from 2 failing tests under
It looks like this is a result of some error messaging/handling changes. Nonetheless, with the state of 'broken' tests currently in
I can't spend too much time on it right now, but happy to do any small changes in my branch if you wish. Any thoughts @peter-rr @Shurtu-gal |
Thanks @aeworxet this issue seems to be resolved through your change. I've noticed a new potential gap, which I've detailed in a comment in my tests PR here: #1389 (comment) |
@francocm @mfroberg
Could you confirm this is the expected bundled file? I'm not sure since the |
@peter-rr {
output: 'YAML',
rules: {
reuseComponents: true,
removeComponents: true,
moveAllToComponents: true,
moveDuplicatesToComponents: false,
},
disableOptimizationFor: {
schema: true,
},
} the output file looks like this: asyncapi: 3.0.0
info:
title: Account Service
version: 1.0.0
description: This service is in charge of processing user signups
channels:
userSignedUp:
$ref: '#/components/channels/userSignedUp'
operations:
onUserSignUp:
$ref: '#/components/operations/onUserSignUp'
components:
messages:
UserSignedUp:
payload:
type: object
properties:
displayName:
type: string
description: Name of the user
email:
type: string
format: email
description: Email of the user
operations:
onUserSignUp:
action: receive
channel:
$ref: '#/channels/userSignedUp'
messages:
- $ref: '#/channels/userSignedUp/messages/UserSignedUp'
channels:
userSignedUp:
address: user/signedup
messages:
UserSignedUp:
$ref: '#/components/messages/UserSignedUp'
Does this suit your needs better? @KhudaDad414 |
@aeworxet currently there is no way to ignore schemas in CLI. I have opened an issue here: #1432 btw, I don't think it's a good solution for the issue that @peter-rr reported. #1323 (comment) The problem is that the bundler de-references the local reference in |
I was going from the v3.0.0 Spec: What else should be changed after this issue is fixed?
Maybe format of
should be rethought in a way that makes both options (in standalone and CLI) consistent? Like
? |
The issue with updating the |
@aeworxet if we leave the optimizer out of the picture for now, which of these options should be the output of the bundler? asyncapi: 3.0.0
info:
title: Account Service
version: 1.0.0
description: This service is in charge of processing user signups
channels:
userSignedUp:
address: user/signedup
messages:
UserSignedUp:
payload:
type: object
properties:
displayName:
type: string
description: Name of the user
email:
type: string
format: email
description: Email of the user
operations:
onUserSignUp:
action: receive
channel:
$ref: '#/channels/userSignedUp'
messages:
- $ref: '#/channels/userSignedUp/messages/UserSignedUp'
components:
messages:
UserSignedUp:
payload:
type: object
properties:
displayName:
type: string
description: Name of the user
email:
type: string
format: email
description: Email of the user asyncapi: 3.0.0
info:
title: Account Service
version: 1.0.0
description: This service is in charge of processing user signups
channels:
userSignedUp:
address: user/signedup
messages:
UserSignedUp:
$ref: '#/components/messages/UserSignedUp'
operations:
onUserSignUp:
action: receive
channel:
$ref: '#/channels/userSignedUp'
messages:
- $ref: '#/channels/userSignedUp/messages/UserSignedUp'
components:
messages:
UserSignedUp:
payload:
type: object
properties:
displayName:
type: string
description: Name of the user
email:
type: string
format: email
description: Email of the user I think option 2 should be the result. as a user of bundler I don't expect bundler to de-reference my local references. If there is disagreement here, I think we should discuss this in a separate issue and see what the community wants.
Agreed. |
I'll ask here since this issue already has the intended audience. Should there be a separate mode in |
I think it is reasonable use case to just dereference external refs and leave local ones intact. My original need is to get rid of the external refs (by bundling that content) so that I am able to visualize the file using Async Studio web tool. |
IMO, that should be the default behaviour. In case we want to de-reference also the local refs, then we could add an option to bundle command for that purpose. |
I will be able to implement |
This issue has been automatically marked as stale because it has not had recent activity 😴 It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation. There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model. Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here. Thank you for your patience ❤️ |
Still relevant. Waiting for #1389 to be solved. |
I'm looking forward to the fix as well. Currently it also kills the https://github.com/event-catalog/eventcatalog asyncapi plugin |
@zbigniew-malcherczyk-tg |
Describe the bug.
The CLI tool now accepts asyncapi bundle for v3 documents (thanks to fix of issue #1137). However, it does not work as I expected, but of course I could be missing something.
I created a new default document with
asyncapi new
and moved the single message to a file in the same directory. Added a $ref in the main file. When I ranasyncapi bundle main.yaml
on the new file, I expected it to merge the “message file”. But it didn’t.main.yaml
messages.yaml
Expected behavior
One yaml file containing the whole specification.
expected_bundle.yaml
Screenshots
How to Reproduce
main.yaml
andmessages.yaml
to the same directoryasyncapi bundle main.yaml
🥦 Browser
None
👀 Have you checked for similar open issues?
🏢 Have you read the Contributing Guidelines?
Are you willing to work on this issue ?
None
The text was updated successfully, but these errors were encountered: