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.

Sign up for GitHub

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

Jump to bottom

fix: add /v1 prefix to the all paths and change 200 response of /generate path #35

Merged
merged 10 commits into from
Jan 18, 2022
Merged

fix: add /v1 prefix to the all paths and change 200 response of /generate path #35

merged 10 commits into from
Jan 18, 2022

Conversation

magicmatatjahu
Copy link
Member

@magicmatatjahu magicmatatjahu commented Jan 17, 2022 •
edited
Loading

Description

As in title:

  • add /v1 prefix to the all paths - as described in the openapi.yaml
  • fix 200 response of /generate path: change application/json to the application/octet-stream.

I don't want to treat is a feat, but as fix, we forgot about that "things" in the previous PRs.

Related issue(s)
Fixes #34

@magicmatatjahu magicmatatjahu added the bug Something isn't working label Jan 17, 2022
@fmvilas
Copy link
Member

fmvilas commented Jan 17, 2022

Isn't it ready for review yet? I see it very much done :)

@magicmatatjahu
Copy link
Member Author

@fmvilas I had to fix tests :)

@magicmatatjahu magicmatatjahu marked this pull request as ready for review January 17, 2022 15:17
@fmvilas
Copy link
Member

fmvilas commented Jan 17, 2022

tests? what's that? :trollface:

fmvilas
fmvilas previously approved these changes Jan 17, 2022
View reviewed changes
@magicmatatjahu
Copy link
Member Author

@fmvilas I see that you have admin privileges to accept PR in this repo, but I would like to wait for review from @smoya or @BOLT04 😄

@fmvilas
Copy link
Member

fmvilas commented Jan 17, 2022

Yeah, I realized after approving. Please just take my approval as a 👍 from a contributor, not a code owner 🙏

smoya
smoya previously approved these changes Jan 17, 2022
View reviewed changes
Copy link
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

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

LGTM!

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Jan 17, 2022 •
edited
Loading

@smoya I also fixed in this PR problem with DockerFile that we know. Could you check again?

smoya
smoya reviewed Jan 17, 2022
View reviewed changes
Dockerfile Outdated

# delete package-lock.json - more info https://github.com/asyncapi/.github/issues/123
# delete that line and install by `npm ci` when mentioned issue will be resolved
RUN rm -rf package-lock.json
Copy link
Member

Choose a reason for hiding this comment

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

Can we create a follow up issue so we can fix this once asyncapi/.github#123 is solved (I hope it does soon)? Removing the package-lock.json file here has unexpected behavior as we don't control which version is gonna be installed of each package if they are not pinned to an specific version...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also against but sometimes switching from v1 to v2 (of packageLock version) can create errors, so bad and so not good 😆 but ok, we can go with that package-lock.json

Copy link
Member Author

Choose a reason for hiding this comment

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

@smoya changed :)

Copy link
Member Author

Choose a reason for hiding this comment

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

And also issue is created #36

Copy link
Member

Choose a reason for hiding this comment

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

Oh no, it's not switching to one or another version something that scares me, no.
The fact we are not downloading exactly the same dependency versions as expected unless they are pinned to an exact version... this scares me 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

@smoya So current changes are ok for you or we should hardcode version of the dependencies?

Copy link
Member

@smoya smoya Jan 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In fact, I think this change should not be added into this PR but to a new one so:

  • We don't hide side effects. This change is totally unexpected according to the name of the final commit that will be merged.
  • We can quickly and easily revert the changes as they will be isolated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted

Copy link
Member Author

Choose a reason for hiding this comment

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

@smoya Created new PR #37

@magicmatatjahu magicmatatjahu changed the title fix: add /v1 prefix to the all paths and fix 200 response of /generate path fix: add /v1 prefix to the all paths, change 200 response of /generate path and fix Dockerfile Jan 17, 2022
@magicmatatjahu magicmatatjahu changed the title fix: add /v1 prefix to the all paths, change 200 response of /generate path and fix Dockerfile fix: add /v1 prefix to the all paths and change 200 response of /generate path Jan 17, 2022
smoya
smoya previously approved these changes Jan 17, 2022
View reviewed changes
BOLT04
BOLT04 reviewed Jan 17, 2022
View reviewed changes
openapi.yaml Outdated Show resolved Hide resolved
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

smoya
smoya approved these changes Jan 17, 2022
View reviewed changes
Copy link
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

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

LGTM!🚀

BOLT04
BOLT04 approved these changes Jan 17, 2022
View reviewed changes
Copy link
Member

@BOLT04 BOLT04 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@magicmatatjahu
Copy link
Member Author

/rtm

@asyncapi-bot asyncapi-bot merged commit 98573a4 into asyncapi:master Jan 18, 2022
@magicmatatjahu magicmatatjahu deleted the bug-fixes branch January 18, 2022 09:10
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BOLT04 BOLT04 BOLT04 approved these changes

@smoya smoya smoya approved these changes

@fmvilas fmvilas fmvilas left review comments

Assignees
No one assigned
Labels
bug Something isn't working ready-to-merge released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OpenAPI: response 200 for /generate is wrong
5 participants
@magicmatatjahu @fmvilas @asyncapi-bot @smoya @BOLT04